Re: [PATCH v18 04/26] drm/shmem-helper: Refactor locked/unlocked functions

[Maxime Ripard]

On Wed, Nov 29, 2023 at 02:46:09PM +0100, Boris Brezillon wrote:
> On Wed, 29 Nov 2023 14:09:47 +0100
> Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> 
> > On Wed, Nov 29, 2023 at 08:53:30AM +0100, Boris Brezillon wrote:
> > > On Wed, 29 Nov 2023 01:05:14 +0300
> > > Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx> wrote:
> > >   
> > > > On 11/28/23 15:37, Boris Brezillon wrote:  
> > > > > On Tue, 28 Nov 2023 12:14:42 +0100
> > > > > Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > > > >     
> > > > >> Hi,
> > > > >>
> > > > >> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:    
> > > > >>> On Fri, 24 Nov 2023 11:40:06 +0100
> > > > >>> Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > > > >>>       
> > > > >>>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:      
> > > > >>>>> Add locked and remove unlocked postfixes from drm-shmem function names,
> > > > >>>>> making names consistent with the drm/gem core code.
> > > > >>>>>
> > > > >>>>> Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > > > >>>>> Suggested-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > > > >>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>        
> > > > >>>>
> > > > >>>> This contradicts my earlier ack on a patch but...
> > > > >>>>       
> > > > >>>>> ---
> > > > >>>>>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> > > > >>>>>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> > > > >>>>>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> > > > >>>>>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> > > > >>>>>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> > > > >>>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> > > > >>>>>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> > > > >>>>>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> > > > >>>>>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> > > > >>>>>  9 files changed, 64 insertions(+), 64 deletions(-)
> > > > >>>>>
> > > > >>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > >>>>> index 0d61f2b3e213..154585ddae08 100644
> > > > >>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > >>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > >>>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> > > > >>>>>  	.pin = drm_gem_shmem_object_pin,
> > > > >>>>>  	.unpin = drm_gem_shmem_object_unpin,
> > > > >>>>>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> > > > >>>>> -	.vmap = drm_gem_shmem_object_vmap,
> > > > >>>>> -	.vunmap = drm_gem_shmem_object_vunmap,
> > > > >>>>> +	.vmap = drm_gem_shmem_object_vmap_locked,
> > > > >>>>> +	.vunmap = drm_gem_shmem_object_vunmap_locked,        
> > > > >>>>
> > > > >>>> While I think we should indeed be consistent with the names, I would
> > > > >>>> also expect helpers to get the locking right by default.      
> > > > >>>
> > > > >>> Wait, actually I think this patch does what you suggest already. The
> > > > >>> _locked() prefix tells the caller: "you should take care of the locking,
> > > > >>> I expect the lock to be held when this is hook/function is called". So
> > > > >>> helpers without the _locked() prefix take care of the locking (which I
> > > > >>> guess matches your 'helpers get the locking right' expectation), and
> > > > >>> those with the _locked() prefix don't.      
> > > > >>
> > > > >> What I meant by "getting the locking right" is indeed a bit ambiguous,
> > > > >> sorry. What I'm trying to say I guess is that, in this particular case,
> > > > >> I don't think you can expect the vmap implementation to be called with
> > > > >> or without the locks held. The doc for that function will say that it's
> > > > >> either one or the other, but not both.
> > > > >>
> > > > >> So helpers should follow what is needed to provide a default vmap/vunmap
> > > > >> implementation, including what locking is expected from a vmap/vunmap
> > > > >> implementation.    
> > > > > 
> > > > > Hm, yeah, I think that's a matter of taste. When locking is often
> > > > > deferrable, like it is in DRM, I find it beneficial for funcions and
> > > > > function pointers to reflect the locking scheme, rather than relying on
> > > > > people properly reading the doc, especially when this is the only
> > > > > outlier in the group of drm_gem_object_funcs we already have, and it's
> > > > > not event documented at the drm_gem_object_funcs level [1] :P.
> > > > >     
> > > > >>
> > > > >> If that means that vmap is always called with the locks taken, then
> > > > >> drm_gem_shmem_object_vmap can just assume that it will be called with
> > > > >> the locks taken and there's no need to mention it in the name (and you
> > > > >> can probably sprinkle a couple of lockdep assertion to make sure the
> > > > >> locking is indeed consistent).    
> > > > > 
> > > > > Things get very confusing when you end up having drm_gem_shmem helpers
> > > > > that are suffixed with _locked() to encode the fact locking is the
> > > > > caller's responsibility and no suffix for the
> > > > > callee-takes-care-of-the-locking semantics, while other helpers that are
> > > > > not suffixed at all actually implement the
> > > > > caller-should-take-care-of-the-locking semantics.
> > > > >     
> > > > >>    
> > > > >>>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> > > > >>>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> > > > >>>> convert whatever function needs to be converted to the unlock suffix so
> > > > >>>> we get a consistent naming.      
> > > > >>>
> > > > >>> That would be an _unlocked() prefix if we do it the other way around. I
> > > > >>> think the main confusion comes from the names of the hooks in
> > > > >>> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
> > > > >>> are called with the GEM resv lock held, and locking is handled by the
> > > > >>> core, others, like drm_gem_shmem_funcs::[un]pin() are called
> > > > >>> without the GEM resv lock held, and locking is deferred to the
> > > > >>> implementation. As I said, I don't mind prefixing hooks/helpers with
> > > > >>> _unlocked() for those that take care of the locking, and no prefix for
> > > > >>> those that expects locks to be held, as long as it's consistent, but I
> > > > >>> just wanted to make sure we're on the same page :-).      
> > > > >>
> > > > >> What about _nolock then? It's the same number of characters than
> > > > >> _locked, plus it expresses what the function is (not) doing, not what
> > > > >> context it's supposed to be called in?    
> > > > > 
> > > > > Just did a quick
> > > > > 
> > > > >   git grep _nolock drivers/gpu/drm
> > > > > 
> > > > > and it returns zero result, where the _locked/_unlocked pattern seems
> > > > > to already be widely used. Not saying we shouldn't change that, but it
> > > > > doesn't feel like a change we should do as part of this series.
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Boris
> > > > > 
> > > > > [1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155    
> > > > 
> > > > I'm fine with dropping the _locked() postfix from the common GEM helpers
> > > > and documenting the locking rule in drm_gem. Thank you all for the
> > > > suggestions :)  
> > > 
> > > Sorry to disagree, but I think a proper function name/suffix is
> > > sometimes worth a few lines of doc. Not saying we should do one or the
> > > other, I think we should do both. But when I see a function suffixed
> > > _locked, _unlocked or _nolock, I can immediately tell if this function
> > > defers the locking to the caller or not, and then go check which lock
> > > in the function doc.
> > > 
> > > And the second thing I'm not happy with, is the fact we go back to an
> > > inconsistent naming in drm_gem_shmem_helper.c, where some functions
> > > deferring the locking to the caller are suffixed _locked and others are
> > > not, because ultimately, you need a different name when you expose the
> > > two variants...  
> > 
> > I guess one of the point I was trying to make was also: why do you need
> > both?
> > 
> > If one is better than the other (whatever better means here), then all
> > drivers should use it.
> > 
> > The counterpart being that if provided a choice, you can be sure that a
> > lot of people will get it wrong. The one example I have in mind for
> > example was the drm_atomic_helper_commit_tail vs
> > drm_atomic_helper_commit_tail_rpm. The latter is now widely used, and
> > most of it is cargo-cult.
> > 
> > I think you were referring to the locks being deferred vs taken right
> > now before, why do we need to have the choice between the two?
>
> Because DRM locking is complex, and you sometimes have to call some
> helpers in a context where you already hold the GEM dma_resv lock.
> That's not the case for _v[un]map(), because the core always takes the
> lock for us if we call drm_gem_vmap_unlocked().

Ok

> Now, let's assume we drop the _locked() suffix on
> drm_gem_shmem_v[un]map(), but keep it on other helpers that need both
> variants. This results in an inconsistent naming scheme inside the
> same source file, which I find utterly confusing.
>
> Note that the initial reason I asked Dmitry if he could add the
> _locked suffix to drm_gem_shmem_vmap() is because I started using
> drm_gem_shmem_vmap() in powervr, before realizing this version wasn't
> taking the lock, and I should have used drm_gem_vmap_unlocked()
> instead, so this is not something I'm making up.

Sorry if I gave you the impression I thought that you're making that up,
I'm not.

Thanks for the explanation btw, I think I get what you're saying now:

 - drm_gem_shmem_vmap() is never taking the locks because the core
   expects to take them before calling them.

 - drm_gem_shmem_vunmap() is never taking the locks because the core
   expects to take them before calling them.

 - Some other code path can still call those helpers in drivers, and the
   locking isn't handled by the core anymore.

 - We now have _vmap/vunmap_unlocked functions to take those locks for
   those code paths

 - And the variant names are now confusing, making people use the
   lockless version in situations where they should have use the locked
   one.

Is that a correct summary?

If so, then I agree that we need to change the name.

We discussed it some more on IRC, and we agree that the "default"
function should handle the locking properly and that's what the most
common case should use.

So that means than drm_gem_shmem_vmap/vunmap() should take the lock
itself, and drm_gem_shmem_vmap/vunmap_nolock/unlocked never does.

I think I'd prefer the nolock variant over unlocked still.

And I also think we can improve the documentation and add lockdep calls
to make sure that the difference between variants is clear in the doc,
and if someone still get confused we can catch it.

Does that sound like a plan?

Maxime

Attachment: signature.asc
Description: PGP signature