Date: Mon, 17 Jul 2023 11:58:55 -0600 From: Warner Losh <imp@bsdimp.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: John Baldwin <jhb@freebsd.org>, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 60381fd1ee86 - main - memdesc: Retire MEMDESC_CCB. Message-ID: <CANCZdfpcbqgEV6Exuj-eGcr9yWuDHcdDvXfK4FZQVZCO8BwcjQ@mail.gmail.com> In-Reply-To: <ZLWARmA8XYh3wR6C@kib.kiev.ua> References: <202307141841.36EIf3f0019403@gitrepo.freebsd.org> <ZLVizUl1Xyo1AQmy@kib.kiev.ua> <65d7d8d8-9f98-abd2-1ce3-ae3a2d3bf111@FreeBSD.org> <CANCZdfoHid=H1Ys_4XTJPfCaifevSoW92nGYXU3Ot=mTT13T%2Bg@mail.gmail.com> <ZLWARmA8XYh3wR6C@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000fc0c310600b28d2b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Jul 17, 2023 at 11:54=E2=80=AFAM Konstantin Belousov <kostikbel@gma= il.com> wrote: > On Mon, Jul 17, 2023 at 11:29:20AM -0600, Warner Losh wrote: > > On Mon, Jul 17, 2023 at 11:15=E2=80=AFAM John Baldwin <jhb@freebsd.org>= wrote: > > > > > On 7/17/23 8:48 AM, Konstantin Belousov wrote: > > > > On Fri, Jul 14, 2023 at 06:41:03PM +0000, John Baldwin wrote: > > > >> The branch main has been updated by jhb: > > > >> > > > >> URL: > > > > https://cgit.FreeBSD.org/src/commit/?id=3D60381fd1ee8668ea1e4676a6128883d= 987cab858 > > > >> > > > >> commit 60381fd1ee8668ea1e4676a6128883d987cab858 > > > >> Author: John Baldwin <jhb@FreeBSD.org> > > > >> AuthorDate: 2023-07-14 18:30:31 +0000 > > > >> Commit: John Baldwin <jhb@FreeBSD.org> > > > >> CommitDate: 2023-07-14 18:32:16 +0000 > > > >> > > > >> memdesc: Retire MEMDESC_CCB. > > > >> > > > >> Instead, change memdesc_ccb to examine the CCB and return a > > > memdesc of > > > >> a more generic type describing the data buffer. > > > > > > > >> diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/subr_bus_dma.c > > > >> index 65a08aeba17c..bfaad30b37d3 100644 > > > >> --- a/sys/kern/subr_bus_dma.c > > > >> +++ b/sys/kern/subr_bus_dma.c > > > >> @@ -304,94 +304,6 @@ bus_dmamap_load_ma_triv(bus_dma_tag_t dmat, > > > bus_dmamap_t map, > > > >> @@ -566,49 +478,18 @@ bus_dmamap_load_ccb(bus_dma_tag_t dmat, > > > bus_dmamap_t map, union ccb *ccb, > > > >> + mem =3D memdesc_ccb(ccb); > > > >> + return (bus_dmamap_load_mem(dmat, map, &mem, callback, > > > callback_arg, > > > >> + flags)); > > > >> } > > > > This makes kernel not linkable if CAM is not included into it. > > > > > > Hmmm, ok. I can either move the memdesc_ccb routine into sys/kern > > > somewhere > > > (like the kern_memdesc.c file in my other pending review), or we can > #ifdef > > > this function. It probably doesn't make sense to have a > > > bus_dmamap_load_ccb > > > if you don't have CAM, so I think I prefer the second option. > > > > > > > MINIMAL doesn't have CAM configured, but it is loadable as a module. > > > > I'd think we'd want a dummy one fo these with weak symbol binding and > have > > the actual > > one live in cam somewhere that overrides this symbol. > The symbol resolution does not work this way in kernel. And it cannot > made working this way even in theory, because cam.ko is loadable at > runtime. > Yea... It could, if we have perfect knowledge of all the places that it is called. But I don't think we do, now that I think about it... so yes, it's good to want things, but in this case my desire cannot exist, I agree. > I believe the only feasible solution is to move memdesc_ccb() into kernel > unconditionally. > Or if it was in cam.h and made a static inline. It's short enough that won't bloat the kernel in the half a dozen places its called, and it would give similar performance to what we have today with the half a dozen nearly identical copies of this routine. And since it's all done with structure dancing, there's no other bits of CAM that would be brought into the kernel. Warner --000000000000fc0c310600b28d2b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Mon, Jul 17, 2023 at 11:54=E2=80= =AFAM Konstantin Belousov <<a href=3D"mailto:kostikbel@gmail.com">kostik= bel@gmail.com</a>> wrote:<br></div><blockquote class=3D"gmail_quote" sty= le=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddi= ng-left:1ex">On Mon, Jul 17, 2023 at 11:29:20AM -0600, Warner Losh wrote:<b= r> > On Mon, Jul 17, 2023 at 11:15=E2=80=AFAM John Baldwin <<a href=3D"m= ailto:jhb@freebsd.org" target=3D"_blank">jhb@freebsd.org</a>> wrote:<br> > <br> > > On 7/17/23 8:48 AM, Konstantin Belousov wrote:<br> > > > On Fri, Jul 14, 2023 at 06:41:03PM +0000, John Baldwin wrote= :<br> > > >> The branch main has been updated by jhb:<br> > > >><br> > > >> URL:<br> > > <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D60381fd1ee86= 68ea1e4676a6128883d987cab858" rel=3D"noreferrer" target=3D"_blank">https://= cgit.FreeBSD.org/src/commit/?id=3D60381fd1ee8668ea1e4676a6128883d987cab858<= /a><br> > > >><br> > > >> commit 60381fd1ee8668ea1e4676a6128883d987cab858<br> > > >> Author:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.= org><br> > > >> AuthorDate: 2023-07-14 18:30:31 +0000<br> > > >> Commit:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@FreeBSD.= org><br> > > >> CommitDate: 2023-07-14 18:32:16 +0000<br> > > >><br> > > >>=C2=A0 =C2=A0 =C2=A0 memdesc: Retire MEMDESC_CCB.<br> > > >><br> > > >>=C2=A0 =C2=A0 =C2=A0 Instead, change memdesc_ccb to exami= ne the CCB and return a<br> > > memdesc of<br> > > >>=C2=A0 =C2=A0 =C2=A0 a more generic type describing the d= ata buffer.<br> > > ><br> > > >> diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/subr_bus= _dma.c<br> > > >> index 65a08aeba17c..bfaad30b37d3 100644<br> > > >> --- a/sys/kern/subr_bus_dma.c<br> > > >> +++ b/sys/kern/subr_bus_dma.c<br> > > >> @@ -304,94 +304,6 @@ bus_dmamap_load_ma_triv(bus_dma_tag= _t dmat,<br> > > bus_dmamap_t map,<br> > > >> @@ -566,49 +478,18 @@ bus_dmamap_load_ccb(bus_dma_tag_t = dmat,<br> > > bus_dmamap_t map, union ccb *ccb,<br> > > >> +=C2=A0 =C2=A0 mem =3D memdesc_ccb(ccb);<br> > > >> +=C2=A0 =C2=A0 return (bus_dmamap_load_mem(dmat, map, &a= mp;mem, callback,<br> > > callback_arg,<br> > > >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 flags));<br> > > >>=C2=A0 =C2=A0}<br> > > > This makes kernel not linkable if CAM is not included into i= t.<br> > ><br> > > Hmmm, ok.=C2=A0 I can either move the memdesc_ccb routine into sy= s/kern<br> > > somewhere<br> > > (like the kern_memdesc.c file in my other pending review), or we = can #ifdef<br> > > this function.=C2=A0 It probably doesn't make sense to have a= <br> > > bus_dmamap_load_ccb<br> > > if you don't have CAM, so I think I prefer the second option.= <br> > ><br> > <br> > MINIMAL doesn't have CAM configured, but it is loadable as a modul= e.<br> > <br> > I'd think we'd want a dummy one fo these with weak symbol bind= ing and have<br> > the actual<br> > one live in cam somewhere that overrides this=C2=A0 symbol.<br> The symbol resolution does not work this way in kernel.=C2=A0 And it cannot= <br> made working this way even in theory, because cam.ko is loadable at<br> runtime.<br></blockquote><div><br></div><div>Yea... It could, if we have pe= rfect knowledge of all the places that it is called.</div><div>But I don= 9;t think we do, now that I think about it... so yes, it's good to want= things,</div><div>but in this case my desire cannot exist, I agree.</div><= div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0= px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> I believe the only feasible solution is to move memdesc_ccb() into kernel<b= r> unconditionally.<br></blockquote><div><br></div><div>Or if it was in cam.h = and made a static inline. It's short enough that won't bloat</div><= div>the kernel in the half a dozen places its called, and it would give sim= ilar performance</div><div>to what we have today with the half a dozen near= ly identical copies of=C2=A0 this routine.</div><div>And since it's all= done with structure dancing, there's no other bits of CAM that would</= div><div>be brought into the kernel.</div><div><br></div><div>Warner</div><= /div></div> --000000000000fc0c310600b28d2b--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpcbqgEV6Exuj-eGcr9yWuDHcdDvXfK4FZQVZCO8BwcjQ>