Re: [PATCH v2 2/2] drm/xe/oa: Fix locking for stream->pollin

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jan 21, 2025 at 08:02:04PM -0800, Ashutosh Dixit wrote:
> Previously locking was skipped for stream->pollin. This "mostly" worked
> because pollin is u32/bool, even when stream->pollin is accessed
> concurrently in multiple threads. However, with stream->pollin moving under
> stream->oa_buffer.ptr_lock in this series, implement the correct way to
> access stream->pollin, which is to access it under
> stream->oa_buffer.ptr_lock.
> 
> v2: Update commit message to explain the "why" of this change (Rodrigo)
>     Document the change in scope for stream->oa_buffer.ptr_lock (Rodrigo)

First of all thanks for the rework.
But I believe I didn't have enough coffee today yet, because
I'm still failing to understand why...

Breaking your explanation to see if I can understand:
'mostly' - Why mostly? Did we face bugs?

'worked because pollin is u32/bool' - this sounds like 'works by luck'

'with stream->pollin moving under stream->oa_buffer.ptr_lock' - Why?

I believe this is the main why that I had yesterday and that continues
today. Why are we using the oa_buffer pointer lock to also protect
the a stream variable?

Why don't you use the stream_lock? Or why don't you create a dedicated
polling_lock?

I'm sorry for not been clear yesterday, wasting your time and 1 cycle...

> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_oa.c       | 6 ++++++
>  drivers/gpu/drm/xe/xe_oa_types.h | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index fa873f3d0a9d1..9de62ce4b9e42 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -530,6 +530,7 @@ static ssize_t xe_oa_read(struct file *file, char __user *buf,
>  			  size_t count, loff_t *ppos)
>  {
>  	struct xe_oa_stream *stream = file->private_data;
> +	unsigned long flags;
>  	size_t offset = 0;
>  	int ret;
>  
> @@ -562,8 +563,10 @@ static ssize_t xe_oa_read(struct file *file, char __user *buf,
>  	 * Also in case of -EIO, we have already waited for data before returning
>  	 * -EIO, so need to wait again
>  	 */
> +	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
>  	if (ret != -ENOSPC && ret != -EIO)
>  		stream->pollin = false;
> +	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
>  
>  	/* Possible values for ret are 0, -EFAULT, -ENOSPC, -EIO, -EINVAL, ... */
>  	return offset ?: (ret ?: -EAGAIN);
> @@ -573,6 +576,7 @@ static __poll_t xe_oa_poll_locked(struct xe_oa_stream *stream,
>  				  struct file *file, poll_table *wait)
>  {
>  	__poll_t events = 0;
> +	unsigned long flags;
>  
>  	poll_wait(file, &stream->poll_wq, wait);
>  
> @@ -582,8 +586,10 @@ static __poll_t xe_oa_poll_locked(struct xe_oa_stream *stream,
>  	 * in use. We rely on hrtimer xe_oa_poll_check_timer_cb to notify us when there
>  	 * are samples to read
>  	 */
> +	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
>  	if (stream->pollin)
>  		events |= EPOLLIN;
> +	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
>  
>  	return events;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_oa_types.h b/drivers/gpu/drm/xe/xe_oa_types.h
> index 52e33c37d5ee8..5c4ea13f646fc 100644
> --- a/drivers/gpu/drm/xe/xe_oa_types.h
> +++ b/drivers/gpu/drm/xe/xe_oa_types.h
> @@ -159,7 +159,7 @@ struct xe_oa_buffer {
>  	/** @vaddr: mapped vaddr of the OA buffer */
>  	u8 *vaddr;
>  
> -	/** @ptr_lock: Lock protecting reads/writes to head/tail pointers */
> +	/** @ptr_lock: Lock protecting reads/writes to head/tail pointers and stream->pollin */
>  	spinlock_t ptr_lock;
>  
>  	/** @head: Cached head to read from */
> -- 
> 2.47.1
>