Re: [PATCH 02/10] drm/vkms: Use spin_lock_irq in process context

From: Daniel Vetter <daniel@ffwll.ch>
To: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Shayenne Moura <shayenneluzmoura@gmail.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 02/10] drm/vkms: Use spin_lock_irq in process context
Date: Wed, 12 Jun 2019 16:54:25 +0200	[thread overview]
Message-ID: <20190612145425.GQ2458@phenom.ffwll.local> (raw)
In-Reply-To: <CADKXj+4HnVEoK7_RVVeicSgxgKe88zTG0HAUR-z1NJdtQs1i4g@mail.gmail.com>

On Wed, Jun 12, 2019 at 10:34:55AM -0300, Rodrigo Siqueira wrote:
> On Thu, Jun 6, 2019 at 7:28 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > The worker is always in process context, no need for the _irqsafe
> > version. Same for the set_source callback, that's only called from the
> > debugfs handler in a syscall.
> >
> > Cc: Shayenne Moura <shayenneluzmoura@gmail.com>
> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  drivers/gpu/drm/vkms/vkms_crc.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c
> > index 66603da634fe..883e36fe7b6e 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crc.c
> > @@ -167,16 +167,15 @@ void vkms_crc_work_handle(struct work_struct *work)
> >         u32 crc32 = 0;
> >         u64 frame_start, frame_end;
> >         bool crc_pending;
> > -       unsigned long flags;
> >
> > -       spin_lock_irqsave(&out->state_lock, flags);
> > +       spin_lock_irq(&out->state_lock);
> >         frame_start = crtc_state->frame_start;
> >         frame_end = crtc_state->frame_end;
> >         crc_pending = crtc_state->crc_pending;
> >         crtc_state->frame_start = 0;
> >         crtc_state->frame_end = 0;
> >         crtc_state->crc_pending = false;
> > -       spin_unlock_irqrestore(&out->state_lock, flags);
> > +       spin_unlock_irq(&out->state_lock);
> >
> >         /*
> >          * We raced with the vblank hrtimer and previous work already computed
> > @@ -246,7 +245,6 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> >  {
> >         struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> >         bool enabled = false;
> > -       unsigned long flags;
> >         int ret = 0;
> >
> >         ret = vkms_crc_parse_source(src_name, &enabled);
> > @@ -254,9 +252,9 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
> >         /* make sure nothing is scheduled on crtc workq */
> >         flush_workqueue(out->crc_workq);
> >
> > -       spin_lock_irqsave(&out->lock, flags);
> > +       spin_lock_irq(&out->lock);
> >         out->crc_enabled = enabled;
> > -       spin_unlock_irqrestore(&out->lock, flags);
> 
> I was wondering if we could use atomic_t for crc_enabled and avoid
> this sort of lock. I did not try to change the data type; this is just
> an idea.

tldr; atomic_t does not do what you think it does. rule of thumb is you
can use it for reference counting and statistics book-keeping, but nothing
else.

The long explanation is that atomic_t in the linux kernel does not have
barriers (unlike atomic values in almost all other language), they are
weakly ordered. If you want to use them for logic (like here with this
bool) you need to think very carefully about barriers, document those
barriers, proof you got it all right, all for maybe a tiny speed-up
(spinlocks are extremely well optimized). In almost all cases that's not
worth it, and fairly often you end up with more atomic operations and so
overall slower code.

btw for this reasons atomic_t is wrapped as refcount_t for those cases
where it's safe to use (plus the code is even more optimized for the
refcount use-case). Except for statistics (like how many crc did we
compute) you shouldn't ever use atomic_t, at least as a good rule of
thumb.
-Daniel

> 
> Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> Tested-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> 
> > +       spin_unlock_irq(&out->lock);
> >
> >         return ret;
> >  }
> > --
> > 2.20.1
> >
> 
> 
> -- 
> 
> Rodrigo Siqueira
> https://siqueira.tech

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel