From nobody Mon Jul 17 18:07:18 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4R4VRR1PDQz4nkWH for ; Mon, 17 Jul 2023 18:07:31 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4R4VRR0rwDz3J4g for ; Mon, 17 Jul 2023 18:07:31 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-ed1-x52f.google.com with SMTP id 4fb4d7f45d1cf-51e48e1f6d1so6472376a12.1 for ; Mon, 17 Jul 2023 11:07:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20221208.gappssmtp.com; s=20221208; t=1689617250; x=1692209250; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=CAiGct4Kwgbcxtc8KYL/QbhmHtpv8pkn7Cp/8NCsYYM=; b=y/9RuUO66DM44vNrHsIWK9nIEs6aH3eOdnAA1B7N3dtHBbZPYsbnE1BvL3HUgw8IQF 0M2V2O3sm8b5qrS9xJPXpuiPY4Yzc6piwLQJ5PXwOK1sHYkXPQfLKFDhKGtnPIey17eu DtfBTO7mM9rBDXkqvFoSSO74OqhEKs600SePu394/XX//3iC5WYFyZ14ja1m4rRIa+7F nbwJfh9ssarVFlWraHZiJ39nmtHvYOyH8LBnCemxHBFOC/6wSmnS71Q+Ng3JBvb+Le84 d1moPtalkx8ttcOf1yencFzUT9th18naaS4VSjSCdXl2//Lryvb3EgHGL2UWjV0cGPpu wrwA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689617250; x=1692209250; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=CAiGct4Kwgbcxtc8KYL/QbhmHtpv8pkn7Cp/8NCsYYM=; b=DKdn8MZWUY0UqNrjSqGLGRuxP8Nofz7GIGIkJKcW4mhV4mihT8SWT2CyTwG22ilyH1 dcNU0Rn6UN2kKAVTMsQY1MUg4fvRA5uv45EEjVAT7pz3cPQEbT1W3ro8cdmOqWnOkb7X ogGpreYN+6tkp5GyoQZ5tn6ccPX9p221d1+nwkQfgN/4lZIUYjA3R9W/zZ7sAK0bRNZr h8zKKlswpLNPnelSg7IK74yeVWdqwygP6JLQZe7/rNx8os2/Ez4PVjxCBixEdwArF8F4 MMbywHaxEpZXXyNuf1+2AzeqZkvnGHXTC1qSgRRaq7+ievKmFvYDQu3iqFCiwEh4Ad12 wEfA== X-Gm-Message-State: ABy/qLb+V4l1tyFF9jz6xGcO2dR7Ao350UeSPzTQwPCDfoA9UFnSLq+E gpxCVLcnonTffckZecNIH9wLaNKy3B9SsVnCXgRyJnB4SnoEgqGTuDs= X-Google-Smtp-Source: APBJJlFAINnF7ilTn4NqEaxSlhCZI7kVqq6AgUodzYt7ctOQxnBiATB7Da92bcx5Wv9KRtMCevOX5swLxujyu+QfZe8= X-Received: by 2002:aa7:d418:0:b0:51e:291b:cc66 with SMTP id z24-20020aa7d418000000b0051e291bcc66mr12367417edq.36.1689617249799; Mon, 17 Jul 2023 11:07:29 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202307141841.36EIf3f0019403@gitrepo.freebsd.org> <65d7d8d8-9f98-abd2-1ce3-ae3a2d3bf111@FreeBSD.org> <92656b28-8bfa-23a9-2683-23ccb817b8a7@FreeBSD.org> In-Reply-To: <92656b28-8bfa-23a9-2683-23ccb817b8a7@FreeBSD.org> From: Warner Losh Date: Mon, 17 Jul 2023 12:07:18 -0600 Message-ID: Subject: Re: git: 60381fd1ee86 - main - memdesc: Retire MEMDESC_CCB. To: John Baldwin Cc: Konstantin Belousov , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="000000000000fb29400600b2abd3" X-Rspamd-Queue-Id: 4R4VRR0rwDz3J4g X-Spamd-Bar: ---- X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-ThisMailContainsUnwantedMimeParts: N --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 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 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 > >>>>>> AuthorDate: 2023-07-14 18:30:31 +0000 > >>>>>> Commit: John Baldwin > >>>>>> 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


=
On Mon, Jul 17, 2023 at 12:02=E2=80= =AFPM John Baldwin <jhb@freebsd.org> wrote:
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.org> wr= ote:
>>>
>>>> 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://c= git.FreeBSD.org/src/commit/?id=3D60381fd1ee8668ea1e4676a6128883d987cab858
>>>>>>
>>>>>> commit 60381fd1ee8668ea1e4676a6128883d987cab858 >>>>>> Author:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@Fr= eeBSD.org>
>>>>>> AuthorDate: 2023-07-14 18:30:31 +0000
>>>>>> Commit:=C2=A0 =C2=A0 =C2=A0John Baldwin <jhb@Fr= eeBSD.org>
>>>>>> CommitDate: 2023-07-14 18:32:16 +0000
>>>>>>
>>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0memdesc: Retire MEMDESC_= CCB.
>>>>>>
>>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0Instead, change memdesc_= ccb to examine the CCB and return a
>>>> memdesc of
>>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0a more generic type desc= ribing the data buffer.
>>>>>
>>>>>> diff --git a/sys/kern/subr_bus_dma.c b/sys/kern/su= br_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_d= ma_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,
>>>>>> +=C2=A0 =C2=A0 mem =3D memdesc_ccb(ccb);
>>>>>> +=C2=A0 =C2=A0 return (bus_dmamap_load_mem(dmat, m= ap, &mem, callback,
>>>> callback_arg,
>>>>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 flags));
>>>>>>=C2=A0 =C2=A0 }
>>>>> This makes kernel not linkable if CAM is not included = into it.
>>>>
>>>> Hmmm, ok.=C2=A0 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.=C2=A0 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 sym= bol binding and
>> have
>>> the actual
>>> one live in cam somewhere that overrides this=C2=A0 symbol. >> The symbol resolution does not work this way in kernel.=C2=A0 And = it cannot
>> made working this way even in theory, because cam.ko is loadable a= t
>> runtime.
>>
>
> Yea... It could, if we have perfect knowledge of all the places that i= t is
> called.
> But I don't think we do, now that I think about it... so yes, it&#= 39;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 si= milar
> 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 oth= er bits of
> CAM that would
> be brought into the kernel.

I would be happy with an inline actually, I wasn't sure originally if t= hat 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 belo= ng there more than cam.h
now that you mention it...=C2=A0 And onl= y sys/kern/subr_bus_dma.c needs it,
since that's the only pla= ce that calls if my grep can be believed.

Warner
--000000000000fb29400600b2abd3--