[Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers

[Dixit, Ashutosh]

On Tue, 31 Mar 2020 23:57:57 -0700, Lionel Landwerlin wrote:
>
> On 01/04/2020 02:14, Ashutosh Dixit wrote:
> > It is wrong to block the user thread in the next poll when OA data is
> > already available which could not fit in the user buffer provided in
> > the previous read. In several cases the exact user buffer size is not
> > known. Blocking user space in poll can lead to data loss when the
> > buffer size used is smaller than the available data.
> >
> > This change fixes this issue and allows user space to read all OA data
> > even when using a buffer size smaller than the available data using
> > multiple non-blocking reads rather than staying blocked in poll till
> > the next timer interrupt.
> >
> > v2: Fix ret value for blocking reads (Umesh)
> > v3: Mistake during patch send (Ashutosh)
> > v4: Remove -EAGAIN from comment (Umesh)
> > v5: Improve condition for clearing pollin and return (Lionel)
> > v6: Improve blocking read loop and other cleanups (Lionel)
> >
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_perf.c | 61 ++++++--------------------------
> >   1 file changed, 11 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 28e3d76fa2e6..2f78b147bb2d 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2963,49 +2963,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
> >		gen8_update_reg_state_unlocked(ce, stream);
> >   }
> >   -/**
> > - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> > - * @stream: An i915 perf stream
> > - * @file: An i915 perf stream file
> > - * @buf: destination buffer given by userspace
> > - * @count: the number of bytes userspace wants to read
> > - * @ppos: (inout) file seek position (unused)
> > - *
> > - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> > - * ensure that if we've successfully copied any data then reporting that takes
> > - * precedence over any internal error status, so the data isn't lost.
> > - *
> > - * For example ret will be -ENOSPC whenever there is more buffered data than
> > - * can be copied to userspace, but that's only interesting if we weren't able
> > - * to copy some data because it implies the userspace buffer is too small to
> > - * receive a single record (and we never split records).
> > - *
> > - * Another case with ret == -EFAULT is more of a grey area since it would seem
> > - * like bad form for userspace to ask us to overrun its buffer, but the user
> > - * knows best:
> > - *
> > - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> > - *
> > - * Returns: The number of bytes copied or a negative error code on failure.
> > - */
> > -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> > -				     struct file *file,
> > -				     char __user *buf,
> > -				     size_t count,
> > -				     loff_t *ppos)
> > -{
> > -	/* Note we keep the offset (aka bytes read) separate from any
> > -	 * error status so that the final check for whether we return
> > -	 * the bytes read with a higher precedence than any error (see
> > -	 * comment below) doesn't need to be handled/duplicated in
> > -	 * stream->ops->read() implementations.
> > -	 */
> > -	size_t offset = 0;
> > -	int ret = stream->ops->read(stream, buf, count, &offset);
> > -
> > -	return offset ?: (ret ?: -EAGAIN);
> > -}
> > -
> >   /**
> >    * i915_perf_read - handles read() FOP for i915 perf stream FDs
> >    * @file: An i915 perf stream file
> > @@ -3031,7 +2988,8 @@ static ssize_t i915_perf_read(struct file *file,
> >   {
> >	struct i915_perf_stream *stream = file->private_data;
> >	struct i915_perf *perf = stream->perf;
> > -	ssize_t ret;
> > +	size_t offset = 0;
> > +	int ret;
> >		/* To ensure it's handled consistently we simply treat all reads of
> > a
> >	 * disabled stream as an error. In particular it might otherwise lead
> > @@ -3054,13 +3012,12 @@ static ssize_t i915_perf_read(struct file *file,
> >				return ret;
> >				mutex_lock(&perf->lock);
> > -			ret = i915_perf_read_locked(stream, file,
> > -						    buf, count, ppos);
> > +			ret = stream->ops->read(stream, buf, count, &offset);
> >			mutex_unlock(&perf->lock);
> > -		} while (ret == -EAGAIN);
> > +		} while (!offset && !ret);
>
> This doesn't sound right, !offset means it will stop as soon as some data
> was written.
>
> But for the blocking read we want to fill the buffer up to -ENOSPC.

I don't think that's true. Here's 'man 2 read': "read() attempts to read
/up to/ count bytes" and "It is not an error if this number is smaller than
the number of bytes requested".

The condition (!offset && !ret) is exactly equivalent to the condition (ret
== -EAGAIN) in the original code (currently on drm-tip). The driver is free
to unblock the blocking read whenever it determines "there is data". Our
determination of "there is data" is "we are woken up by the OA timer and
call ops->read() and offset > 0". (Offset will be equal to min(amount of
data available, space in the user buffer)). The only constraint seems to be
that the blocking read cannot return -EAGAIN (0 bytes) and the loop in the
code guards against that.

> while (ret >= 0) doesn't work?

Because this is not the logic in the original code and I see no reason to
change that logic. It will also change the blocking read behavior which
according to some people is a breakage of the uAPI. The purpose of the
patch is to fix the non blocking read path (poll + non-blocking read). It
should not affect blocking read imo.

Thanks!
--
Ashutosh