On Tue, 28 Jan 2025, Chuck Lever wrote:
> On 1/26/25 5:33 PM, NeilBrown wrote:
> > On Fri, 24 Jan 2025, Chuck Lever wrote:
> >> On 1/22/25 5:10 PM, NeilBrown wrote:
> >>> On Thu, 23 Jan 2025, Chuck Lever wrote:
> >>>> On 1/21/25 10:54 PM, NeilBrown wrote:
> >>>>> The final freeing of nfsd files is done by per-net nfsd threads (which
> >>>>> call nfsd_file_net_dispose()) so it makes some sense to make more of the
> >>>>> freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
> >>>>>
> >>>>> This is a step towards replacing the list_lru with simple lists which
> >>>>> each share the per-net lock in nfsd_fcache_disposal and will require
> >>>>> less list walking.
> >>>>>
> >>>>> As the net is always shutdown before there is any chance that the rest
> >>>>> of the filecache is shut down we can removed the tests on
> >>>>> NFSD_FILE_CACHE_UP.
> >>>>>
> >>>>> For the filecache stats file, which assumes a global lru, we keep a
> >>>>> separate counter which includes all files in all netns lrus.
> >>>>
> >>>> Hey Neil -
> >>>>
> >>>> One of the issues with the current code is that the contention for
> >>>> the LRU lock has been effectively buried. It would be nice to have a
> >>>> way to expose that contention in the new code.
> >>>>
> >>>> Can this patch or this series add some lock class infrastructure to
> >>>> help account for the time spent contending for these dynamically
> >>>> allocated spin locks?
> >>>
> >>> Unfortunately I don't know anything about accounting for lock contention
> >>> time.
> >>>
> >>> I know about lock classes for the purpose of lockdep checking but not
> >>> how they relate to contention tracking.
> >>> Could you give me some pointers?
> >>
> >> Sticking these locks into a class is all you need to do. When lockstat
> >> is enabled, it automatically accumulates the statistics for all locks
> >> in a class, and treats that as a single lock in /proc/lock_stat.
> >>
> >
> > Ahh thanks. So I don't need to do anything as this lock has it's own
> > spin_lock_init() so it already has it's own class.
> > However.... the init call is :
> >
> > spin_lock_init(&l->lock);
> >
> > so that appear in /proc/lock_stat as
> >
> > &l->lock:
> > or maybe
> > &l->lock/1:
> > or even
> > &l->lock/2:
>
> Well, that's the problem, as I see it. They might all appear separately,
> but we want lock_stat to group them together so we can see the aggregate
> wait time and contention metrics.
That isn't what is happening here.
$ git grep 'spin_lock_init(&l->lock)'
fs/bcachefs/nocow_locking.c: spin_lock_init(&l->lock);
fs/nfsd/filecache.c: spin_lock_init(&l->lock);
kernel/bpf/bpf_lru_list.c: raw_spin_lock_init(&l->lock);
mm/list_lru.c: spin_lock_init(&l->lock);
so there are 4 completely different locks that can appear in
/proc/locks_stat as &l->lock. So subsequent ones are given numeric
suffixes.
>
> Have a look at svc_reclassify_socket().
Ahh - nice.
I was thinking it would be good to have
spin_lock_init_name()
for cases like this. Maybe I should post a patch..
Thanks,
NeilBrown
>
>
> > Maybe I should change the "l" to something more unique. "nfd" ??
> > Or I could actually define a lock_class_key and call __spin_lock_init:
> >
> > __skin_lock_init(&l->lock, "nfsd_fcache_disposal->lock", &key,
> > false);
> >
> > There is no precedent for that though.
> >
> > NeilBrown
> >
>
>
> --
> Chuck Lever
>
