Re: [dpdk-dev] [PATCH 20.11] raw/ioat: added a flag to control copying handle parameters

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Jul 13, 2020 at 10:55:30AM +0100, Bruce Richardson wrote:
> On Mon, Jul 13, 2020 at 07:15:19AM +0000, Cheng Jiang wrote:
> > Added a flag which controls whether rte_ioat_enqueue_copy
> > and rte_ioat_completed_copies function should process
> > handle parameters to improve the performance when handle
> > parameters are not necessary to use. This is targeting
> > 20.11 release.
> > 
> > Signed-off-by: Cheng Jiang <Cheng1.jiang@intel.com>
> > ---
> >  drivers/raw/ioat/ioat_rawdev.c     |  1 +
> >  drivers/raw/ioat/rte_ioat_rawdev.h | 14 +++++++++++---
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> <snip>
> > @@ -208,6 +213,11 @@ rte_ioat_completed_copies(int dev_id, uint8_t max_copies,
> >  	if (count > max_copies)
> >  		count = max_copies;
> >  
> > +	ioat->next_read = read + count;
> > +	ioat->completed += count;
> > +	if (!ioat->hdls_enable)
> > +		return count;
> > +
> >  	for (; i < count - 1; i += 2, read += 2) {
> >  		__m128i hdls0 = _mm_load_si128(&ioat->hdls[read & mask]);
> >  		__m128i hdls1 = _mm_load_si128(&ioat->hdls[(read + 1) & mask]);
> > @@ -223,8 +233,6 @@ rte_ioat_completed_copies(int dev_id, uint8_t max_copies,
> >  		dst_hdls[i] = hdls[1];
> >  	}
> >  
> > -	ioat->next_read = read;
> > -	ioat->completed += count;
> >  	return count;
> >  }
> 
> This change I think may cause problems if we ever want to have one thread
> enqueuing and another taking completions. The next_read and completed
> counters should really only be updated after we have finished reading the
> completed handles array. Therefore, for safety, I tihnk it might be better
> to keep the updates in their original places and put an "end:" label before
> them. Then the "return count" in the middle of the function can be "goto
> end;"
> 
A further suggestion to the changes to this function: if we are not
actually returning completion handles, then there is no need to limit the
count to "max_copies". Therefore move the "return count" or "goto end"
above the previous length check, and update the doxygen comments to
indicate that max_copies is ignored if "hdls_enable" is false, and that the
final two parameters can also be NULL when calling the function in this
case.

/Bruce