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
[-- Attachment #1 --] On Mon, Jul 17, 2023 at 11:54 AM Konstantin Belousov <kostikbel@gmail.com> wrote: > On Mon, Jul 17, 2023 at 11:29:20AM -0600, Warner Losh wrote: > > On Mon, Jul 17, 2023 at 11:15 AM 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=60381fd1ee8668ea1e4676a6128883d987cab858 > > > >> > > > >> 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 = 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 [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 17, 2023 at 11:54 AM Konstantin Belousov <<a href="mailto:kostikbel@gmail.com">kostikbel@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Jul 17, 2023 at 11:29:20AM -0600, Warner Losh wrote:<br> > On Mon, Jul 17, 2023 at 11:15 AM John Baldwin <<a href="mailto:jhb@freebsd.org" target="_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="https://cgit.FreeBSD.org/src/commit/?id=60381fd1ee8668ea1e4676a6128883d987cab858" rel="noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=60381fd1ee8668ea1e4676a6128883d987cab858</a><br>; > > >><br> > > >> commit 60381fd1ee8668ea1e4676a6128883d987cab858<br> > > >> Author: John Baldwin <jhb@FreeBSD.org><br> > > >> AuthorDate: 2023-07-14 18:30:31 +0000<br> > > >> Commit: John Baldwin <jhb@FreeBSD.org><br> > > >> CommitDate: 2023-07-14 18:32:16 +0000<br> > > >><br> > > >> memdesc: Retire MEMDESC_CCB.<br> > > >><br> > > >> Instead, change memdesc_ccb to examine the CCB and return a<br> > > memdesc of<br> > > >> a more generic type describing the data 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> > > >> + mem = memdesc_ccb(ccb);<br> > > >> + return (bus_dmamap_load_mem(dmat, map, &mem, callback,<br> > > callback_arg,<br> > > >> + flags));<br> > > >> }<br> > > > This makes kernel not linkable if CAM is not included into it.<br> > ><br> > > Hmmm, ok. I can either move the memdesc_ccb routine into sys/kern<br> > > somewhere<br> > > (like the kern_memdesc.c file in my other pending review), or we can #ifdef<br> > > this function. 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 module.<br> > <br> > I'd think we'd want a dummy one fo these with weak symbol binding and have<br> > the actual<br> > one live in cam somewhere that overrides this symbol.<br> The symbol resolution does not work this way in kernel. 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 perfect knowledge of all the places that it is called.</div><div>But I don'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> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 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<br> 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 similar performance</div><div>to what we have today with the half a dozen nearly identical copies of 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>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpcbqgEV6Exuj-eGcr9yWuDHcdDvXfK4FZQVZCO8BwcjQ>