Re: [v2 PATCH 7/9] mm: vmscan: don't need allocate shrinker->nr_deferred for memcg aware shrinkers
On Thu, Dec 17, 2020 at 04:56:48PM -0800, Yang Shi wrote: > On Tue, Dec 15, 2020 at 3:07 PM Yang Shi 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. -- Dave Chinner da...@fromorbit.com
Re: [v2 PATCH 7/9] mm: vmscan: don't need allocate shrinker->nr_deferred for memcg aware shrinkers
On Tue, Dec 15, 2020 at 3:07 PM Yang Shi wrote:
>
> On Mon, Dec 14, 2020 at 7:05 PM Dave Chinner 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
> > > ---
> > > 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
> > da...@fromorbit.com
Re: [v2 PATCH 7/9] mm: vmscan: don't need allocate shrinker->nr_deferred for memcg aware shrinkers
On Mon, Dec 14, 2020 at 7:05 PM Dave Chinner 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
> > ---
> > 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
> da...@fromorbit.com
Re: [v2 PATCH 7/9] mm: vmscan: don't need allocate shrinker->nr_deferred for memcg aware shrinkers
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
> ---
> 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.
--
Dave Chinner
da...@fromorbit.com
[v2 PATCH 7/9] mm: vmscan: don't need allocate shrinker->nr_deferred for memcg aware shrinkers
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
---
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;
}
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;
--
2.26.2
