Re: git: 60381fd1ee86 - main - memdesc: Retire MEMDESC_CCB.
Date: Mon, 17 Jul 2023 18:02:13 UTC
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? -- John Baldwin