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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 22 Jan 2025 02:23:23 -0800, Rodrigo Vivi wrote:
>

Hi Rodrigo,

Thanks for your inputs, it helped me look at the code more closely and
clarify some things.

I have responded to your comments below, but in case you don't want to go
through that, the summary is that I would like to drop this patch from
consideration at this time, basically because it needs more changes.

> 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'

No, there are no known bugs in the current code in the kernel. That is what
'mostly' is supposed to mean: basically the requirements on this code are
pretty lax, and even if writes into the boolean stream->pollin from
different threads cross each other, or they stomp on each other, things
work out. So not 'works by luck' or 'works most of the time' but the code
works even in absence of this locking.

>
> '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?
>

So, stream->oa_buffer.ptr_lock _is_ the correct lock to use for
'stream->pollin', because 'stream->pollin' is intimately connected to the
same pointers 'ptr_lock' is protecting. You can see this in Patch 1.

Also, there is basically one 'struct xe_oa_buffer' per stream (and stream_lock
is a mutex which can't be used from interrupt context).

So rather than using a stream level lock, we can just move pollin into
stream->oa_buffer. So it will become stream->oa_buffer.pollin.

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

No, thanks for your comments, it made me think about all this.

> > @@ -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);

Also this is the part I don't like, this should be moved into
xe_oa_append_reports() where stream->oa_buffer.ptr_lock is already
held. Otherwise we are dropping and re-grabbing the lock etc.

So imo the basic direction of this patch is correct but I want to make all
these changes and resubmit this as part of a separate series. So please
ignore this patch for now. I will go ahead and merge Patch 1 since that is
ok and was already reviewed as a separate series:

https://patchwork.freedesktop.org/series/143575/

Thanks.
--
Ashutosh



> >
> >	/* 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
> >