Re: git: 60381fd1ee86 - main - memdesc: Retire MEMDESC_CCB.

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