| Message ID | 20201214223722.232537-8-shy828301@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make shrinker's nr_deferred memcg aware | expand |
On Mon, Dec 14, 2020 at 02:37:20PM -0800, Yang Shi wrote: > Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need > allocate shrinker->nr_deferred for such shrinkers anymore. > > Signed-off-by: Yang Shi <shy828301@gmail.com> > --- > mm/vmscan.c | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index bce8cf44eca2..8d5bfd818acd 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -420,7 +420,15 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone > */ > int prealloc_shrinker(struct shrinker *shrinker) > { > - unsigned int size = sizeof(*shrinker->nr_deferred); > + unsigned int size; > + > + if (is_deferred_memcg_aware(shrinker)) { > + if (prealloc_memcg_shrinker(shrinker)) > + return -ENOMEM; > + return 0; > + } > + > + size = sizeof(*shrinker->nr_deferred); > > if (shrinker->flags & SHRINKER_NUMA_AWARE) > size *= nr_node_ids; > @@ -429,26 +437,18 @@ int prealloc_shrinker(struct shrinker *shrinker) > if (!shrinker->nr_deferred) > return -ENOMEM; > > - if (shrinker->flags & SHRINKER_MEMCG_AWARE) { > - if (prealloc_memcg_shrinker(shrinker)) > - goto free_deferred; > - } > - > return 0; > - > -free_deferred: > - kfree(shrinker->nr_deferred); > - shrinker->nr_deferred = NULL; > - return -ENOMEM; > } I'm trying to put my finger on it, but this seems wrong to me. If memcgs are disabled, then prealloc_memcg_shrinker() needs to fail. The preallocation code should not care about internal memcg details like this. /* * If the shrinker is memcg aware and memcgs are not * enabled, clear the MEMCG flag and fall back to non-memcg * behaviour for the shrinker. */ if (shrinker->flags & SHRINKER_MEMCG_AWARE) { error = prealloc_memcg_shrinker(shrinker); if (!error) return 0; if (error != -ENOSYS) return error; /* memcgs not enabled! */ shrinker->flags &= ~SHRINKER_MEMCG_AWARE; } size = sizeof(*shrinker->nr_deferred); .... return 0; } This guarantees that only the shrinker instances taht have a correctly set up memcg attached to them will have the SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker code, we only ever need to check for SHRINKER_MEMCG_AWARE to determine what we should do.... > void free_prealloced_shrinker(struct shrinker *shrinker) > { > - if (!shrinker->nr_deferred) > + if (is_deferred_memcg_aware(shrinker)) { > + unregister_memcg_shrinker(shrinker); > return; > + } > > - if (shrinker->flags & SHRINKER_MEMCG_AWARE) > - unregister_memcg_shrinker(shrinker); > + if (!shrinker->nr_deferred) > + return; > > kfree(shrinker->nr_deferred); > shrinker->nr_deferred = NULL; e.g. then this function can simply do: { if (shrinker->flags & SHRINKER_MEMCG_AWARE) return unregister_memcg_shrinker(shrinker); kfree(shrinker->nr_deferred); shrinker->nr_deferred = NULL; } Cheers, Dave.
On Mon, Dec 14, 2020 at 7:05 PM Dave Chinner <david@fromorbit.com> wrote: > > On Mon, Dec 14, 2020 at 02:37:20PM -0800, Yang Shi wrote: > > Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need > > allocate shrinker->nr_deferred for such shrinkers anymore. > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > --- > > mm/vmscan.c | 28 ++++++++++++++-------------- > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index bce8cf44eca2..8d5bfd818acd 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -420,7 +420,15 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone > > */ > > int prealloc_shrinker(struct shrinker *shrinker) > > { > > - unsigned int size = sizeof(*shrinker->nr_deferred); > > + unsigned int size; > > + > > + if (is_deferred_memcg_aware(shrinker)) { > > + if (prealloc_memcg_shrinker(shrinker)) > > + return -ENOMEM; > > + return 0; > > + } > > + > > + size = sizeof(*shrinker->nr_deferred); > > > > if (shrinker->flags & SHRINKER_NUMA_AWARE) > > size *= nr_node_ids; > > @@ -429,26 +437,18 @@ int prealloc_shrinker(struct shrinker *shrinker) > > if (!shrinker->nr_deferred) > > return -ENOMEM; > > > > - if (shrinker->flags & SHRINKER_MEMCG_AWARE) { > > - if (prealloc_memcg_shrinker(shrinker)) > > - goto free_deferred; > > - } > > - > > return 0; > > - > > -free_deferred: > > - kfree(shrinker->nr_deferred); > > - shrinker->nr_deferred = NULL; > > - return -ENOMEM; > > } > > I'm trying to put my finger on it, but this seems wrong to me. If > memcgs are disabled, then prealloc_memcg_shrinker() needs to fail. > The preallocation code should not care about internal memcg details > like this. > > /* > * If the shrinker is memcg aware and memcgs are not > * enabled, clear the MEMCG flag and fall back to non-memcg > * behaviour for the shrinker. > */ > if (shrinker->flags & SHRINKER_MEMCG_AWARE) { > error = prealloc_memcg_shrinker(shrinker); > if (!error) > return 0; > if (error != -ENOSYS) > return error; > > /* memcgs not enabled! */ > shrinker->flags &= ~SHRINKER_MEMCG_AWARE; > } > > size = sizeof(*shrinker->nr_deferred); > .... > return 0; > } > > This guarantees that only the shrinker instances taht have a > correctly set up memcg attached to them will have the > SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker > code, we only ever need to check for SHRINKER_MEMCG_AWARE to > determine what we should do.... Thanks. I see your point. We could move the memcg specific details into prealloc_memcg_shrinker(). It seems we have to acquire shrinker_rwsem before we check and modify SHIRNKER_MEMCG_AWARE bit if we may clear it. > > > void free_prealloced_shrinker(struct shrinker *shrinker) > > { > > - if (!shrinker->nr_deferred) > > + if (is_deferred_memcg_aware(shrinker)) { > > + unregister_memcg_shrinker(shrinker); > > return; > > + } > > > > - if (shrinker->flags & SHRINKER_MEMCG_AWARE) > > - unregister_memcg_shrinker(shrinker); > > + if (!shrinker->nr_deferred) > > + return; > > > > kfree(shrinker->nr_deferred); > > shrinker->nr_deferred = NULL; > > e.g. then this function can simply do: > > { > if (shrinker->flags & SHRINKER_MEMCG_AWARE) > return unregister_memcg_shrinker(shrinker); > kfree(shrinker->nr_deferred); > shrinker->nr_deferred = NULL; > } > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Tue, Dec 15, 2020 at 3:07 PM Yang Shi <shy828301@gmail.com> wrote: > > On Mon, Dec 14, 2020 at 7:05 PM Dave Chinner <david@fromorbit.com> wrote: > > > > On Mon, Dec 14, 2020 at 02:37:20PM -0800, Yang Shi wrote: > > > Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need > > > allocate shrinker->nr_deferred for such shrinkers anymore. > > > > > > Signed-off-by: Yang Shi <shy828301@gmail.com> > > > --- > > > mm/vmscan.c | 28 ++++++++++++++-------------- > > > 1 file changed, 14 insertions(+), 14 deletions(-) > > > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > > index bce8cf44eca2..8d5bfd818acd 100644 > > > --- a/mm/vmscan.c > > > +++ b/mm/vmscan.c > > > @@ -420,7 +420,15 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone > > > */ > > > int prealloc_shrinker(struct shrinker *shrinker) > > > { > > > - unsigned int size = sizeof(*shrinker->nr_deferred); > > > + unsigned int size; > > > + > > > + if (is_deferred_memcg_aware(shrinker)) { > > > + if (prealloc_memcg_shrinker(shrinker)) > > > + return -ENOMEM; > > > + return 0; > > > + } > > > + > > > + size = sizeof(*shrinker->nr_deferred); > > > > > > if (shrinker->flags & SHRINKER_NUMA_AWARE) > > > size *= nr_node_ids; > > > @@ -429,26 +437,18 @@ int prealloc_shrinker(struct shrinker *shrinker) > > > if (!shrinker->nr_deferred) > > > return -ENOMEM; > > > > > > - if (shrinker->flags & SHRINKER_MEMCG_AWARE) { > > > - if (prealloc_memcg_shrinker(shrinker)) > > > - goto free_deferred; > > > - } > > > - > > > return 0; > > > - > > > -free_deferred: > > > - kfree(shrinker->nr_deferred); > > > - shrinker->nr_deferred = NULL; > > > - return -ENOMEM; > > > } > > > > I'm trying to put my finger on it, but this seems wrong to me. If > > memcgs are disabled, then prealloc_memcg_shrinker() needs to fail. > > The preallocation code should not care about internal memcg details > > like this. > > > > /* > > * If the shrinker is memcg aware and memcgs are not > > * enabled, clear the MEMCG flag and fall back to non-memcg > > * behaviour for the shrinker. > > */ > > if (shrinker->flags & SHRINKER_MEMCG_AWARE) { > > error = prealloc_memcg_shrinker(shrinker); > > if (!error) > > return 0; > > if (error != -ENOSYS) > > return error; > > > > /* memcgs not enabled! */ > > shrinker->flags &= ~SHRINKER_MEMCG_AWARE; > > } > > > > size = sizeof(*shrinker->nr_deferred); > > .... > > return 0; > > } > > > > This guarantees that only the shrinker instances taht have a > > correctly set up memcg attached to them will have the > > SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker > > code, we only ever need to check for SHRINKER_MEMCG_AWARE to > > determine what we should do.... > > Thanks. I see your point. We could move the memcg specific details > into prealloc_memcg_shrinker(). > > It seems we have to acquire shrinker_rwsem before we check and modify > SHIRNKER_MEMCG_AWARE bit if we may clear it. Hi Dave, Is it possible that shrinker register races with shrinker unregister? It seems impossible to me by a quick visual code inspection. But I'm not a VFS expert so I'm not quite sure. If it is impossible the implementation would be quite simple otherwise we need move shrinker_rwsem acquire/release to prealloc_shrinker/free_prealloced_shrinker/unregister_shrinker to protect SHRINKER_MEMCG_AWARE update. > > > > > > void free_prealloced_shrinker(struct shrinker *shrinker) > > > { > > > - if (!shrinker->nr_deferred) > > > + if (is_deferred_memcg_aware(shrinker)) { > > > + unregister_memcg_shrinker(shrinker); > > > return; > > > + } > > > > > > - if (shrinker->flags & SHRINKER_MEMCG_AWARE) > > > - unregister_memcg_shrinker(shrinker); > > > + if (!shrinker->nr_deferred) > > > + return; > > > > > > kfree(shrinker->nr_deferred); > > > shrinker->nr_deferred = NULL; > > > > e.g. then this function can simply do: > > > > { > > if (shrinker->flags & SHRINKER_MEMCG_AWARE) > > return unregister_memcg_shrinker(shrinker); > > kfree(shrinker->nr_deferred); > > shrinker->nr_deferred = NULL; > > } > > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@fromorbit.com
On Thu, Dec 17, 2020 at 04:56:48PM -0800, Yang Shi wrote: > On Tue, Dec 15, 2020 at 3:07 PM Yang Shi <shy828301@gmail.com> wrote: > > > This guarantees that only the shrinker instances taht have a > > > correctly set up memcg attached to them will have the > > > SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker > > > code, we only ever need to check for SHRINKER_MEMCG_AWARE to > > > determine what we should do.... > > > > Thanks. I see your point. We could move the memcg specific details > > into prealloc_memcg_shrinker(). > > > > It seems we have to acquire shrinker_rwsem before we check and modify > > SHIRNKER_MEMCG_AWARE bit if we may clear it. > > Hi Dave, > > Is it possible that shrinker register races with shrinker unregister? > It seems impossible to me by a quick visual code inspection. But I'm > not a VFS expert so I'm not quite sure. Uh, if you have a shrinker racing to register and unregister, you've got a major bug in your object initialisation/teardown code. i.e. calling reagister/unregister at the same time for the same shrinker is a bug, pure and simple. Cheers, Dave.
diff --git a/mm/vmscan.c b/mm/vmscan.c index bce8cf44eca2..8d5bfd818acd 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -420,7 +420,15 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone */ int prealloc_shrinker(struct shrinker *shrinker) { - unsigned int size = sizeof(*shrinker->nr_deferred); + unsigned int size; + + if (is_deferred_memcg_aware(shrinker)) { + if (prealloc_memcg_shrinker(shrinker)) + return -ENOMEM; + return 0; + } + + size = sizeof(*shrinker->nr_deferred); if (shrinker->flags & SHRINKER_NUMA_AWARE) size *= nr_node_ids; @@ -429,26 +437,18 @@ int prealloc_shrinker(struct shrinker *shrinker) if (!shrinker->nr_deferred) return -ENOMEM; - if (shrinker->flags & SHRINKER_MEMCG_AWARE) { - if (prealloc_memcg_shrinker(shrinker)) - goto free_deferred; - } - return 0; - -free_deferred: - kfree(shrinker->nr_deferred); - shrinker->nr_deferred = NULL; - return -ENOMEM; } void free_prealloced_shrinker(struct shrinker *shrinker) { - if (!shrinker->nr_deferred) + if (is_deferred_memcg_aware(shrinker)) { + unregister_memcg_shrinker(shrinker); return; + } - if (shrinker->flags & SHRINKER_MEMCG_AWARE) - unregister_memcg_shrinker(shrinker); + if (!shrinker->nr_deferred) + return; kfree(shrinker->nr_deferred); shrinker->nr_deferred = NULL;
Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need allocate shrinker->nr_deferred for such shrinkers anymore. Signed-off-by: Yang Shi <shy828301@gmail.com> --- mm/vmscan.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)