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
--000000000000fb29400600b2abd3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Jul 17, 2023 at 12:02=E2=80=AFPM John Baldwin <jhb@freebsd.org> wro= te: > On 7/17/23 10:58 AM, Warner Losh wrote: > > On Mon, Jul 17, 2023 at 11:54=E2=80=AFAM 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=E2=80=AFAM John Baldwin <jhb@freebsd.or= g> 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 o= f > > CAM that would > > be brought into the kernel. > > I would be happy with an inline actually, I wasn't sure originally if tha= t > 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 --000000000000fb29400600b2abd3 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 12:02=E2=80= =AFPM John Baldwin <<a href=3D"mailto:jhb@freebsd.org">jhb@freebsd.org</= a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0p= x 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=E2=80=AFAM Konstantin Belousov <<a hr= ef=3D"mailto:kostikbel@gmail.com" target=3D"_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=E2=80=AFAM John Baldwin <<a h= ref=3D"mailto:jhb@freebsd.org" target=3D"_blank">jhb@freebsd.org</a>> wr= ote:<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=3D"https://cgit.FreeBSD.org/src/commit/?id=3D60381fd1ee866= 8ea1e4676a6128883d987cab858" rel=3D"noreferrer" target=3D"_blank">https://c= git.FreeBSD.org/src/commit/?id=3D60381fd1ee8668ea1e4676a6128883d987cab858</= a><br> >>>>>><br> >>>>>> commit 60381fd1ee8668ea1e4676a6128883d987cab858<br= > >>>>>> Author:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@Fr= eeBSD.org><br> >>>>>> AuthorDate: 2023-07-14 18:30:31 +0000<br> >>>>>> Commit:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@Fr= eeBSD.org><br> >>>>>> CommitDate: 2023-07-14 18:32:16 +0000<br> >>>>>><br> >>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0memdesc: Retire MEMDESC_= CCB.<br> >>>>>><br> >>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0Instead, change memdesc_= ccb to examine the CCB and return a<br> >>>> memdesc of<br> >>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0a more generic type desc= ribing the data buffer.<br> >>>>><br> >>>>>> diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/su= br_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_d= ma_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, m= ap, &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 it.<br> >>>><br> >>>> Hmmm, ok.=C2=A0 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.=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 module.<br> >>><br> >>> I'd think we'd want a dummy one fo these with weak sym= bol binding and<br> >> 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 a= t<br> >> runtime.<br> >><br> > <br> > Yea... It could, if we have perfect knowledge of all the places that i= t is<br> > called.<br> > But I don't think we do, now that I think about it... so yes, it&#= 39;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 si= milar<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 oth= er 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 t= hat 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></blockquo= te><div><br></div><div>cam_ccb.h likely is fine, and logically it does belo= ng there more than cam.h</div><div>now that you mention it...=C2=A0 And onl= y sys/kern/subr_bus_dma.c needs it,</div><div>since that's the only pla= ce that calls if my grep can be believed.</div><div><br></div><div>Warner</= div></div></div> --000000000000fb29400600b2abd3--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfoFR7%2BZpcMTc1rnYDGc-KM98=a0-U0Zp_4eEqmzwSPemA>