svn commit: r354703 - head/sys/dev/ioat

[Konstantin Belousov]

On Fri, Nov 15, 2019 at 11:09:12AM -0700, Scott Long wrote:
> 
> 
> > On Nov 15, 2019, at 2:18 AM, Konstantin Belousov <kostikbel at gmail.com> wrote:
> > 
> > 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
> >>>> 
> >>>> Log:
> >>>>  Pass more reasonable WAIT flags to bus_dma(9) calls.
> >>>> 
> >>>>  MFC after:	2 weeks
> >>>> 
> >>>> Modified:
> >>>>  head/sys/dev/ioat/ioat.c
> >>>> 
> >>>> Modified: head/sys/dev/ioat/ioat.c
> >>>> ==============================================================================
> >>>> --- 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);
> >>>> 
> >>>> 	error = 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.
> >> 
> >> 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.
> > 
> > I see.  Still, if you (or somebody else) ever decided to use WAITOK loads, 
> > it would be much safer to provide the lock function now.
> > 
> 
> 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’s the caller’s 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’t need to be held.
I only mean that if waitable loads are to be used, now or in future, then
the locking function should be supplied.  Also I mean that it is easy to
forget to supply it because bounce busdma on amd64 and modern DMA engines
never delay loading.

> 
> It’s 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’s change is correct.
> 
> Scott
>