[Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Thu Mar 26 18:02:46 UTC 2020
On Wed, Mar 25, 2020 at 06:52:52PM -0700, Dixit, Ashutosh wrote:
>On Wed, 25 Mar 2020 17:32:35 -0700, Umesh Nerlige Ramappa wrote:
>>
>> On Wed, Mar 25, 2020 at 11:20:19AM -0700, 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.
>> >
>> > 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 | 62 ++++++--------------------------
>> > 1 file changed, 11 insertions(+), 51 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> > index 3222f6cd8255..c1a47c030941 100644
>> > --- a/drivers/gpu/drm/i915/i915_perf.c
>> > +++ b/drivers/gpu/drm/i915/i915_perf.c
>> > @@ -2957,49 +2957,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
>> > @@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
>> > {
>> > struct i915_perf_stream *stream = file->private_data;
>> > struct i915_perf *perf = stream->perf;
>> > + size_t offset = 0;
>> > + int __ret;
>> > ssize_t ret;
>> >
>> > /* To ensure it's handled consistently we simply treat all reads of a
>> > @@ -3048,16 +3007,18 @@ 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);
>>
>> ret will never be EAGAIN here in the while. EAGAIN was returned by the
>> deleted function in this patch if offset and ret are both 0.
>
>Good catch, I was so focussed on the non-blocking case that I missed the
>blocking case.
>
>> Although I don't see how that would be true.
>
>As you say above, the old function i915_perf_read_locked() was doing this:
>
> return offset ?: (__ret ?: -EAGAIN);
>
>So -EAGAIN is returned from i915_perf_read_locked() when there is no data
>to read but otherwise there is no other error. Since this is blocking read
>we cannot return -EAGAIN to user space (since there is no data to read), we
>must go back and block again. That is the purpose of the while loop. I
>broke this logic in this patch and will need to fix this.
>
>>
>> > } else {
>> > 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);
>> > }
>> >
>> > + /* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */
>>
>> __ret may never be EAGAIN either (comment^). I don't see EAGAIN in the read
>> path.
>
>It's here:
>
>gen8_append_oa_reports()
>{
>
> /*
> * An invalid tail pointer here means we're still waiting for the poll
> * hrtimer callback to give us a pointer
> */
> if (tail == INVALID_TAIL_PTR)
> return -EAGAIN;
>}
Oh, you are right, EAGAIN is returned here. I was looking for it with
the poll period patch series applied and these references are removed in
that series.
Thanks,
Umesh
>
>> That said, EAGAIN seems to have been introduced in the prior code
>> specifically for retrying the blocking read and may not have much meaning
>> otherwise.
>
>No that's not true. The kernel non-blocking read() function (in fops)
>returns -EAGAIN when there is no data to read (the function never returns 0
>except in case of EOF, in i915 perf code there is no EOF so read never
>returns 0). This logic is the same as that in the previous code and we need
>to preserve it.
>
>Will post a v2 with the fix.
>
>Thanks!
>--
>Ashutosh
>
>
>>
>> Thanks,
>> Umesh
>>
>> > + ret = offset ?: (__ret ?: -EAGAIN);
>> > +
>> > /* We allow the poll checking to sometimes report false positive EPOLLIN
>> > * events where we might actually report EAGAIN on read() if there's
>> > * not really any data available. In this situation though we don't
>> > @@ -3065,13 +3026,12 @@ static ssize_t i915_perf_read(struct file *file,
>> > * and read() returning -EAGAIN. Clearing the oa.pollin state here
>> > * effectively ensures we back off until the next hrtimer callback
>> > * before reporting another EPOLLIN event.
>> > + * The exception to this is if ops->read() returned -ENOSPC which means
>> > + * that more OA data is available than could fit in the user provided
>> > + * buffer. In this case we want the next poll() call to not block.
>> > */
>> > - if (ret >= 0 || ret == -EAGAIN) {
>> > - /* Maybe make ->pollin per-stream state if we support multiple
>> > - * concurrent streams in the future.
>> > - */
>> > + if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
>> > stream->pollin = false;
>> > - }
>> >
>> > return ret;
>> > }
>> > --
>> > 2.25.2
>> >
More information about the Intel-gfx
mailing list