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

[Lionel Landwerlin]

On 01/04/2020 10:43, Dixit, Ashutosh wrote:
> 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

Ah sorry, you're right :)


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>