Re: [PATCH 4/7] drm/lease: Check for lessor outside of locks

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 3 Apr 2019 09:04:03 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Mar 14, 2019 at 09:07:25AM +0100, Boris Brezillon wrote:
> > On Thu, 28 Feb 2019 15:49:07 +0100
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >   
> > > The lessor is invariant over a lifetime of a lease, we don't have to
> > > grab any locks for that. Speeds up the common case of not being a lease.
> > > 
> > > Cc: Keith Packard <keithp@keithp.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/drm_lease.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> > > index cce5d9dd52ff..694ff363a90b 100644
> > > --- a/drivers/gpu/drm/drm_lease.c
> > > +++ b/drivers/gpu/drm/drm_lease.c
> > > @@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master *master, int id)
> > >   */
> > >  bool _drm_lease_held(struct drm_file *file_priv, int id)
> > >  {
> > > -	if (file_priv == NULL || file_priv->master == NULL)
> > > +	if (!file_priv || !file_priv->master)  
> > 
> > Looks like you're doing unrelated cosmetic changes in the same patch.
> > Maybe mention that in the commit message, or move that to a separate
> > patch.  
> 
> The next hunk would have broken the 80 limit with the == NULL check, so I
> changed those for consistency too because ocd. I can mention that in the
> commit message.

Okay.

> >   
> > >  		return true;
> > >  
> > >  	return _drm_lease_held_master(file_priv->master, id);
> > > @@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
> > >  	struct drm_master *master;
> > >  	bool ret;
> > >  
> > > -	if (file_priv == NULL || file_priv->master == NULL)
> > > +	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> > >  		return true;
> > >  
> > >  	master = file_priv->master;
> > > @@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
> > >  	int count_in, count_out;
> > >  	uint32_t crtcs_out = 0;
> > >  
> > > -	if (file_priv == NULL || file_priv->master == NULL)
> > > +	if (!file_priv || !file_priv->master || !file_priv->master->lessor)
> > >  		return crtcs_in;
> > >  
> > >  	master = file_priv->master;  
> > 
> > Couldn't we also remove the if (master->lessor) check done in
> > _drm_lease_held_master()?  
> 
> How? For !master->lessor (which is the case for all top-level masters,
> i.e. one created by opening the /dev node and not through the create lease
> ioctl) there's no lease idr. These are handled by the unconditional return
> true. And there's some call chains leading to this which don't first check
> for master->lessor (the object find stuff through _drm_lease_held).

I had the impression that all callers of _drm_lease_held_master() were
now doing the necessary checks to guarantee that master->lessor == NULL
cannot happen in _drm_lease_held_master(). But it seems that
_drm_lease_held() and drm_lease_create() do not check the value of
master->lessor.

> 
> Also confused by "also remove", this patch doesn't drop any checks, just
> adds them outside of the lock to extend existing fastpaths.

Yes, I meant "remove" not "also remove". Sorry for the confusion.

FWIW, here is my

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel