Date: Fri, 15 Nov 2019 11:09:12 -0700 From: Scott Long <scottl@samsco.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Alexander Motin <mav@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r354703 - head/sys/dev/ioat Message-ID: <842A5858-39B8-4196-8A94-018FA37DE75E@samsco.org> In-Reply-To: <20191115091837.GD2707@kib.kiev.ua> References: <201911140439.xAE4dngZ032224@repo.freebsd.org> <20191114101149.GA2707@kib.kiev.ua> <475757c6-3707-540b-0316-cbef278043c2@FreeBSD.org> <20191115091837.GD2707@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Nov 15, 2019, at 2:18 AM, Konstantin Belousov <kostikbel@gmail.com> = wrote: >=20 > On Thu, Nov 14, 2019 at 09:41:36AM -0500, Alexander Motin wrote: >> On 14.11.2019 05:11, Konstantin Belousov wrote: >>> On Thu, Nov 14, 2019 at 04:39:49AM +0000, Alexander Motin wrote: >>>> Author: mav >>>> Date: Thu Nov 14 04:39:48 2019 >>>> New Revision: 354703 >>>> URL: https://svnweb.freebsd.org/changeset/base/354703 >>>>=20 >>>> Log: >>>> Pass more reasonable WAIT flags to bus_dma(9) calls. >>>>=20 >>>> MFC after: 2 weeks >>>>=20 >>>> Modified: >>>> head/sys/dev/ioat/ioat.c >>>>=20 >>>> Modified: head/sys/dev/ioat/ioat.c >>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>> --- head/sys/dev/ioat/ioat.c Thu Nov 14 04:34:58 2019 = (r354702) >>>> +++ head/sys/dev/ioat/ioat.c Thu Nov 14 04:39:48 2019 = (r354703) >>>> @@ -555,13 +555,14 @@ ioat3_attach(device_t device) >>>> &ioat->comp_update_tag); >>>>=20 >>>> error =3D bus_dmamem_alloc(ioat->comp_update_tag, >>>> - (void **)&ioat->comp_update, BUS_DMA_ZERO, = &ioat->comp_update_map); >>>> + (void **)&ioat->comp_update, BUS_DMA_ZERO | BUS_DMA_WAITOK, >>>> + &ioat->comp_update_map); >>> For waitok, you need to provide locking function in the tag. >>=20 >> I'm sorry, but why? It is alloc(), not load(). For load() it makes >> sense since it calls back, but this is just a form of malloc(), isn't >> it? I've looked through the both x86 implementations and found = nothing >> suspicious. >=20 > I see. Still, if you (or somebody else) ever decided to use WAITOK = loads,=20 > it would be much safer to provide the lock function now. >=20 Loads are always non-blocking, and the WAITOK flag is not part of that API. A load may defer its callback, and the asynchronous execution of = that callback is why we have the loaned lock. If a blocking alloc is = performed in a context where the caller holds a lock, then it=E2=80=99s the = caller=E2=80=99s responsibility to indicate NOWAIT and deal with the possible failure. Just like with malloc and contigmalloc, the API does not, nor will it ever, drop and require locks on behalf of the caller. The API tried to enforce the = practice that static resource allocation should happen at initialization time = when locks don=E2=80=99t need to be held. It=E2=80=99s unfortunate that the NOWAIT flag is overloaded for = different meanings in the load functions vs the alloc functions. Maybe this is what is = causing confusion? TL;DR: ALexander=E2=80=99s change is correct. Scott
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?842A5858-39B8-4196-8A94-018FA37DE75E>