diff mbox series

[1/1,SRU,Focal] drm/i915/perf: Do not clear pollin for small user read buffers

Message ID 20200506025004.630079-2-acelan.kao@canonical.com
State New
Headers show
Series Dell XPS 13 9300 mirror mode doesn't work sometimes with WD19TB | expand

Commit Message

AceLan Kao May 6, 2020, 2:50 a.m. UTC
From: Ashutosh Dixit <ashutosh.dixit@intel.com>

BugLink: https://bugs.launchpad.net/bugs/1877013

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)
v7: Added Cc stable

Testcase: igt/perf/polling-small-buf
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20200403010120.3067-1-ashutosh.dixit@intel.com
(cherry-picked from commit 6352219c39c04ed3f9a8d1cf93f87c21753a213e)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
(backported from commit bcad588dea538a4fc173d16a90a005536ec8dbf2)
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 65 ++++++--------------------------
 1 file changed, 11 insertions(+), 54 deletions(-)

Comments

Kleber Sacilotto de Souza May 13, 2020, 9:30 a.m. UTC | #1
On 06.05.20 04:50, AceLan Kao wrote:
> From: Ashutosh Dixit <ashutosh.dixit@intel.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1877013
> 
> 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)
> v7: Added Cc stable
> 
> Testcase: igt/perf/polling-small-buf
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Link: https://patchwork.freedesktop.org/patch/msgid/20200403010120.3067-1-ashutosh.dixit@intel.com
> (cherry-picked from commit 6352219c39c04ed3f9a8d1cf93f87c21753a213e)
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> (backported from commit bcad588dea538a4fc173d16a90a005536ec8dbf2)
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>

This patch seems to have been applied to 5.5.y and 5.6.y only, so I'm wondering
why it hasn't been applied to 5.4.y which is LTS.

Anyway, this is a clean cherry-pick and we have good test results.


Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>

> ---
>  drivers/gpu/drm/i915/i915_perf.c | 65 ++++++--------------------------
>  1 file changed, 11 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index e42b86827d6b..bf917fc1837d 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2312,49 +2312,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>  		gen8_update_reg_state_unlocked(stream, ce, regs, stream->oa_config);
>  }
>  
> -/**
> - * 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
> @@ -2380,7 +2337,8 @@ static ssize_t i915_perf_read(struct file *file,
>  {
>  	struct i915_perf_stream *stream = file->private_data;
>  	struct drm_i915_private *dev_priv = stream->dev_priv;
> -	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
> @@ -2403,13 +2361,12 @@ static ssize_t i915_perf_read(struct file *file,
>  				return ret;
>  
>  			mutex_lock(&dev_priv->perf.lock);
> -			ret = i915_perf_read_locked(stream, file,
> -						    buf, count, ppos);
> +			ret = stream->ops->read(stream, buf, count, &offset);
>  			mutex_unlock(&dev_priv->perf.lock);
> -		} while (ret == -EAGAIN);
> +		} while (!offset && !ret);
>  	} else {
>  		mutex_lock(&dev_priv->perf.lock);
> -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> +		ret = stream->ops->read(stream, buf, count, &offset);
>  		mutex_unlock(&dev_priv->perf.lock);
>  	}
>  
> @@ -2420,15 +2377,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) {
> -		/* Maybe make ->pollin per-stream state if we support multiple
> -		 * concurrent streams in the future.
> -		 */
> +	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)
>
Stefan Bader May 13, 2020, 9:42 a.m. UTC | #2
On 13.05.20 11:30, Kleber Souza wrote:
> On 06.05.20 04:50, AceLan Kao wrote:
>> From: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1877013
>>
>> 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)
>> v7: Added Cc stable
>>
>> Testcase: igt/perf/polling-small-buf
>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Link: https://patchwork.freedesktop.org/patch/msgid/20200403010120.3067-1-ashutosh.dixit@intel.com
>> (cherry-picked from commit 6352219c39c04ed3f9a8d1cf93f87c21753a213e)
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> (backported from commit bcad588dea538a4fc173d16a90a005536ec8dbf2)
>> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
> 
> This patch seems to have been applied to 5.5.y and 5.6.y only, so I'm wondering
> why it hasn't been applied to 5.4.y which is LTS.
> 
> Anyway, this is a clean cherry-pick and we have good test results.
> 
> 
> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> 
>> ---

I can only guess that the answer is that it does not apply cleanly (thus was
backported to focal). I can not see obvious differences and with good testing
results it seems acceptable.

>>  drivers/gpu/drm/i915/i915_perf.c | 65 ++++++--------------------------
>>  1 file changed, 11 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index e42b86827d6b..bf917fc1837d 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -2312,49 +2312,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>>  		gen8_update_reg_state_unlocked(stream, ce, regs, stream->oa_config);
>>  }
>>  
>> -/**
>> - * 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
>> @@ -2380,7 +2337,8 @@ static ssize_t i915_perf_read(struct file *file,
>>  {
>>  	struct i915_perf_stream *stream = file->private_data;
>>  	struct drm_i915_private *dev_priv = stream->dev_priv;
>> -	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
>> @@ -2403,13 +2361,12 @@ static ssize_t i915_perf_read(struct file *file,
>>  				return ret;
>>  
>>  			mutex_lock(&dev_priv->perf.lock);
>> -			ret = i915_perf_read_locked(stream, file,
>> -						    buf, count, ppos);
>> +			ret = stream->ops->read(stream, buf, count, &offset);
>>  			mutex_unlock(&dev_priv->perf.lock);
>> -		} while (ret == -EAGAIN);
>> +		} while (!offset && !ret);
>>  	} else {
>>  		mutex_lock(&dev_priv->perf.lock);
>> -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
>> +		ret = stream->ops->read(stream, buf, count, &offset);
>>  		mutex_unlock(&dev_priv->perf.lock);
>>  	}
>>  
>> @@ -2420,15 +2377,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) {
>> -		/* Maybe make ->pollin per-stream state if we support multiple
>> -		 * concurrent streams in the future.
>> -		 */
>> +	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)
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index e42b86827d6b..bf917fc1837d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2312,49 +2312,6 @@  void i915_oa_init_reg_state(struct intel_engine_cs *engine,
 		gen8_update_reg_state_unlocked(stream, ce, regs, stream->oa_config);
 }
 
-/**
- * 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
@@ -2380,7 +2337,8 @@  static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct drm_i915_private *dev_priv = stream->dev_priv;
-	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
@@ -2403,13 +2361,12 @@  static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&dev_priv->perf.lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			ret = stream->ops->read(stream, buf, count, &offset);
 			mutex_unlock(&dev_priv->perf.lock);
-		} while (ret == -EAGAIN);
+		} while (!offset && !ret);
 	} else {
 		mutex_lock(&dev_priv->perf.lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		ret = stream->ops->read(stream, buf, count, &offset);
 		mutex_unlock(&dev_priv->perf.lock);
 	}
 
@@ -2420,15 +2377,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) {
-		/* Maybe make ->pollin per-stream state if we support multiple
-		 * concurrent streams in the future.
-		 */
+	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)