From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
To: Ashutosh Dixit <ashutosh.dixit@intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
Date: Tue, 31 Mar 2020 10:34:10 +0300 [thread overview]
Message-ID: <ec657de2-e38a-c27c-ef08-3a12e34aaa2a@intel.com> (raw)
In-Reply-To: <20200331052222.76525-1-ashutosh.dixit@intel.com>
On 31/03/2020 08:22, 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)
>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
I forgot to mention this needs to be Cc: stable.
Still one nit below which should make the remaining function a bit simpler.
Thanks for your time.
-Lionel
> ---
> drivers/gpu/drm/i915/i915_perf.c | 62 +++++++-------------------------
> 1 file changed, 13 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index c74ebac50015..9c21f28f89a7 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2914,49 +2914,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
> @@ -2982,7 +2939,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
> @@ -2992,6 +2950,8 @@ static ssize_t i915_perf_read(struct file *file,
> return -EIO;
>
> if (!(file->f_flags & O_NONBLOCK)) {
> + ssize_t ret;
> +
> /* There's the small chance of false positives from
> * stream->ops->wait_unlocked.
> *
> @@ -3005,13 +2965,13 @@ 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);
I think you can keep using ret and just change the loop to while (ret >=
0) (which means no failure).
You will get -ENOSPC when the whole buffer is filled or some other error
which should trigger stream closure.
Finally you can 0 if nothing was written but there was nothing to read
and that keeps the read going.
> + ret = offset ?: (__ret ?: -EAGAIN);
> mutex_unlock(&perf->lock);
> } while (ret == -EAGAIN);
> } 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);
> }
>
> @@ -3022,11 +2982,15 @@ 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)
> + if (__ret != -ENOSPC)
> stream->pollin = false;
>
> - return ret;
> + /* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
> + return offset ?: (__ret ?: -EAGAIN);
> }
>
> static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-03-31 7:34 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-31 5:22 [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Ashutosh Dixit
2020-03-31 6:04 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: Do not clear pollin for small user read buffers (rev5) Patchwork
2020-03-31 7:34 ` Lionel Landwerlin [this message]
2020-03-31 23:29 ` [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Dixit, Ashutosh
2020-03-31 14:06 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/perf: Do not clear pollin for small user read buffers (rev5) Patchwork
-- strict thread matches above, loose matches on Subject: below --
2020-04-08 23:42 [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Ashutosh Dixit
2020-04-13 22:05 ` Rodrigo Vivi
2020-04-03 1:01 Ashutosh Dixit
2020-04-03 16:17 ` Chris Wilson
2020-04-03 17:45 ` Dixit, Ashutosh
2020-04-03 17:59 ` Chris Wilson
2020-03-31 23:14 Ashutosh Dixit
2020-04-01 6:57 ` Lionel Landwerlin
2020-04-01 7:43 ` Dixit, Ashutosh
2020-04-01 7:51 ` Lionel Landwerlin
2020-03-27 23:16 Ashutosh Dixit
2020-03-30 8:23 ` Lionel Landwerlin
2020-03-31 5:47 ` Dixit, Ashutosh
2020-03-27 23:11 Ashutosh Dixit
2020-03-26 4:43 Ashutosh Dixit
2020-03-26 9:09 ` Lionel Landwerlin
2020-03-27 3:39 ` Dixit, Ashutosh
2020-03-25 18:20 Ashutosh Dixit
2020-03-25 19:25 ` Lionel Landwerlin
2020-03-25 20:44 ` Dixit, Ashutosh
2020-03-26 0:32 ` Umesh Nerlige Ramappa
2020-03-26 1:52 ` Dixit, Ashutosh
2020-03-26 18:02 ` Umesh Nerlige Ramappa
2020-03-27 1:28 ` Dixit, Ashutosh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ec657de2-e38a-c27c-ef08-3a12e34aaa2a@intel.com \
--to=lionel.g.landwerlin@intel.com \
--cc=ashutosh.dixit@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).