Date: Mon, 17 Jul 2023 12:07:18 -0600 From: Warner Losh <imp@bsdimp.com> To: John Baldwin <jhb@freebsd.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, 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: <CANCZdfoFR7%2BZpcMTc1rnYDGc-KM98=a0-U0Zp_4eEqmzwSPemA@mail.gmail.com> In-Reply-To: <92656b28-8bfa-23a9-2683-23ccb817b8a7@FreeBSD.org> 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> <CANCZdfpcbqgEV6Exuj-eGcr9yWuDHcdDvXfK4FZQVZCO8BwcjQ@mail.gmail.com> <92656b28-8bfa-23a9-2683-23ccb817b8a7@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] On Mon, Jul 17, 2023 at 12:02 PM John Baldwin <jhb@freebsd.org> wrote: > On 7/17/23 10:58 AM, Warner Losh wrote: > > 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. > > I would be happy with an inline actually, I wasn't sure originally if that > was > too invasive in terms of the header bloat it would entail, in particular > if it > lived in sys/memdesc.h, but maybe it could live in cam_ccb.h? > cam_ccb.h likely is fine, and logically it does belong there more than cam.h now that you mention it... And only sys/kern/subr_bus_dma.c needs it, since that's the only place that calls if my grep can be believed. 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 12:02 PM John Baldwin <<a href="mailto:jhb@freebsd.org">jhb@freebsd.org</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 7/17/23 10:58 AM, Warner Losh wrote:<br> > On Mon, Jul 17, 2023 at 11:54 AM Konstantin Belousov <<a href="mailto:kostikbel@gmail.com" target="_blank">kostikbel@gmail.com</a>><br> > wrote:<br> > <br> >> 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> >>>><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<br> >> #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<br> >> 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> >><br> > <br> > Yea... It could, if we have perfect knowledge of all the places that it is<br> > called.<br> > But I don't think we do, now that I think about it... so yes, it's good to<br> > want things,<br> > but in this case my desire cannot exist, I agree.<br> > <br> > <br> >> I believe the only feasible solution is to move memdesc_ccb() into kernel<br> >> unconditionally.<br> >><br> > <br> > Or if it was in cam.h and made a static inline. It's short enough that<br> > won't bloat<br> > the kernel in the half a dozen places its called, and it would give similar<br> > performance<br> > to what we have today with the half a dozen nearly identical copies of<br> > this routine.<br> > And since it's all done with structure dancing, there's no other bits of<br> > CAM that would<br> > be brought into the kernel.<br> <br> I would be happy with an inline actually, I wasn't sure originally if that was<br> too invasive in terms of the header bloat it would entail, in particular if it<br> lived in sys/memdesc.h, but maybe it could live in cam_ccb.h?<br></blockquote><div><br></div><div>cam_ccb.h likely is fine, and logically it does belong there more than cam.h</div><div>now that you mention it... And only sys/kern/subr_bus_dma.c needs it,</div><div>since that's the only place that calls if my grep can be believed.</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?CANCZdfoFR7%2BZpcMTc1rnYDGc-KM98=a0-U0Zp_4eEqmzwSPemA>