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

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

On Tue, 21 Jan 2025 15:19:03 -0800, Rodrigo Vivi wrote:
>

Hi Rodrigo,

> On Tue, Jan 21, 2025 at 10:03:54AM -0800, Ashutosh Dixit wrote:
> > Previously locking was not implemented for stream->pollin. Now
> > stream->pollin should be accessed under stream->oa_buffer.ptr_lock.
>
> This commit message fails to explain why. Why it was not needed
> before and why it is needed now?

I've sent a v2 and explained this in the v2 commit message.

> Also, please make sure that the lock really makes sense and
> we are not just increasing the scope of a locking that was
> designed for something else...

It is increasing the scope of a previous lock, but to me it looks ok after
the change introduced in Patch 1 of this series. IMO it is better to
increase the scope of one previous lock rather then introduce yet another
lock. I have also updated the comment for the lock to reflect this change
in v2.

> and that the locking guidelines are followed... [1]
>
> [1] - https://blog.ffwll.ch/2022/07/locking-engineering.html

Locking guidelines are followed, it is just a spinlock protecting
concurrent data modification. I've also checked there are no ABBA lock
inversion issues: when multiple locks are taken they are always taken by
taking stream->stream_lock first, followed by stream->oa_buffer.ptr_lock.

Thanks.
--
Ashutosh



> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > 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;
> >  }
> > --
> > 2.47.1
> >