Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Tue, 8 Aug 2023 15:22:51 +0800 | Subject | Re: [PATCH v4 45/48] mm: shrinker: make global slab shrink lockless | From | Qi Zheng <> |
| |
Hi Dave,
On 2023/8/8 10:24, Dave Chinner wrote: > On Mon, Aug 07, 2023 at 07:09:33PM +0800, Qi Zheng wrote: >> diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h >> index eb342994675a..f06225f18531 100644 >> --- a/include/linux/shrinker.h >> +++ b/include/linux/shrinker.h >> @@ -4,6 +4,8 @@ >> >> #include <linux/atomic.h> >> #include <linux/types.h> >> +#include <linux/refcount.h> >> +#include <linux/completion.h> >> >> #define SHRINKER_UNIT_BITS BITS_PER_LONG >> >> @@ -87,6 +89,10 @@ struct shrinker { >> int seeks; /* seeks to recreate an obj */ >> unsigned flags; >> >> + refcount_t refcount; >> + struct completion done; >> + struct rcu_head rcu; > > Documentation, please. What does the refcount protect, what does the > completion provide, etc.
How about the following:
/* * reference count of this shrinker, holding this can guarantee * that the shrinker will not be released. */ refcount_t refcount; /* * Wait for shrinker::refcount to reach 0, that is, no shrinker * is running or will run again. */ struct completion done;
> >> + >> void *private_data; >> >> /* These are for internal use */ >> @@ -120,6 +126,17 @@ struct shrinker *shrinker_alloc(unsigned int flags, const char *fmt, ...); >> void shrinker_register(struct shrinker *shrinker); >> void shrinker_free(struct shrinker *shrinker); >> >> +static inline bool shrinker_try_get(struct shrinker *shrinker) >> +{ >> + return refcount_inc_not_zero(&shrinker->refcount); >> +} >> + >> +static inline void shrinker_put(struct shrinker *shrinker) >> +{ >> + if (refcount_dec_and_test(&shrinker->refcount)) >> + complete(&shrinker->done); >> +} >> + >> #ifdef CONFIG_SHRINKER_DEBUG >> extern int __printf(2, 3) shrinker_debugfs_rename(struct shrinker *shrinker, >> const char *fmt, ...); >> diff --git a/mm/shrinker.c b/mm/shrinker.c >> index 1911c06b8af5..d318f5621862 100644 >> --- a/mm/shrinker.c >> +++ b/mm/shrinker.c >> @@ -2,6 +2,7 @@ >> #include <linux/memcontrol.h> >> #include <linux/rwsem.h> >> #include <linux/shrinker.h> >> +#include <linux/rculist.h> >> #include <trace/events/vmscan.h> >> >> #include "internal.h" >> @@ -577,33 +578,42 @@ unsigned long shrink_slab(gfp_t gfp_mask, int nid, struct mem_cgroup *memcg, >> if (!mem_cgroup_disabled() && !mem_cgroup_is_root(memcg)) >> return shrink_slab_memcg(gfp_mask, nid, memcg, priority); >> >> - if (!down_read_trylock(&shrinker_rwsem)) >> - goto out; >> - >> - list_for_each_entry(shrinker, &shrinker_list, list) { >> + rcu_read_lock(); >> + list_for_each_entry_rcu(shrinker, &shrinker_list, list) { >> struct shrink_control sc = { >> .gfp_mask = gfp_mask, >> .nid = nid, >> .memcg = memcg, >> }; >> >> + if (!shrinker_try_get(shrinker)) >> + continue; >> + >> + /* >> + * We can safely unlock the RCU lock here since we already >> + * hold the refcount of the shrinker. >> + */ >> + rcu_read_unlock(); >> + >> ret = do_shrink_slab(&sc, shrinker, priority); >> if (ret == SHRINK_EMPTY) >> ret = 0; >> freed += ret; >> + >> /* >> - * Bail out if someone want to register a new shrinker to >> - * prevent the registration from being stalled for long periods >> - * by parallel ongoing shrinking. >> + * This shrinker may be deleted from shrinker_list and freed >> + * after the shrinker_put() below, but this shrinker is still >> + * used for the next traversal. So it is necessary to hold the >> + * RCU lock first to prevent this shrinker from being freed, >> + * which also ensures that the next shrinker that is traversed >> + * will not be freed (even if it is deleted from shrinker_list >> + * at the same time). >> */ > > This needs to be moved to the head of the function, and document > the whole list walk, get, put and completion parts of the algorithm > that make it safe. There's more to this than "we hold a reference > count", especially the tricky "we might see the shrinker before it > is fully initialised" case....
How about moving these documents to before list_for_each_entry_rcu(), and then go to the head of shrink_slab_memcg() to explain the memcg slab shrink case.
> > > ..... >> void shrinker_free(struct shrinker *shrinker) >> { >> struct dentry *debugfs_entry = NULL; >> @@ -686,9 +712,18 @@ void shrinker_free(struct shrinker *shrinker) >> if (!shrinker) >> return; >> >> + if (shrinker->flags & SHRINKER_REGISTERED) { >> + shrinker_put(shrinker); >> + wait_for_completion(&shrinker->done); >> + } > > Needs a comment explaining why we need to wait here...
/* * Wait for all lookups of the shrinker to complete, after that, no * shrinker is running or will run again, then we can safely free * the structure where the shrinker is located, such as super_block * etc. */
>> + >> down_write(&shrinker_rwsem); >> if (shrinker->flags & SHRINKER_REGISTERED) { >> - list_del(&shrinker->list); >> + /* >> + * Lookups on the shrinker are over and will fail in the future, >> + * so we can now remove it from the lists and free it. >> + */ > > .... rather than here after the wait has been done and provided the > guarantee that no shrinker is running or will run again...
With the above comment, how about simplifying the comment here to the following:
/* * Now we can safely remove it from the shrinker_list and free it. */ Thanks, Qi
> > -Dave.
| ![\](/images/icornerr.gif) |