| Message ID | 20220312071623.19050-2-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] mm/memcg: mz already removed from rb_tree in mem_cgroup_largest_soft_limit_node() | expand |
On Sat 12-03-22 07:16:22, Wei Yang wrote: > There is no tree operation if mz is not on-tree. This doesn't explain problem you are trying to solve nor does it make much sense to me TBH. > Let's remove the extra check. What would happen if the mz was already in the excess tree and the excess has grown? > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > --- > mm/memcontrol.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index d70bf5cf04eb..344a7e891bc5 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -545,9 +545,11 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, int nid) > unsigned long flags; > > spin_lock_irqsave(&mctz->lock, flags); > - /* if on-tree, remove it */ > - if (mz->on_tree) > - __mem_cgroup_remove_exceeded(mz, mctz); > + /* > + * remove it first > + * If not on-tree, no tree ops. > + */ > + __mem_cgroup_remove_exceeded(mz, mctz); > /* > * Insert again. mz->usage_in_excess will be updated. > * If excess is 0, no tree ops. > -- > 2.33.1
On Mon, Mar 14, 2022 at 10:54:03AM +0100, Michal Hocko wrote: >On Sat 12-03-22 07:16:22, Wei Yang wrote: >> There is no tree operation if mz is not on-tree. > >This doesn't explain problem you are trying to solve nor does it make >much sense to me TBH. > This just tries to make the code looks consistent. >> Let's remove the extra check. > >What would happen if the mz was already in the excess tree and the >excess has grown? The purpose mem_cgroup_update_tree() is to update the soft limit tree. And the approach is to remove and add it back to the tree with new excess. I don't get your point for this question.
On Mon 14-03-22 22:51:50, Wei Yang wrote: > On Mon, Mar 14, 2022 at 10:54:03AM +0100, Michal Hocko wrote: > >On Sat 12-03-22 07:16:22, Wei Yang wrote: > >> There is no tree operation if mz is not on-tree. > > > >This doesn't explain problem you are trying to solve nor does it make > >much sense to me TBH. > > > > This just tries to make the code looks consistent. I guess this is rather subjective. One could argue that the check is more descriptive because it obviously removes the node from the tree when it is on the tree.
On Tue, Mar 15, 2022 at 4:52 PM Michal Hocko <mhocko@suse.com> wrote: > > On Mon 14-03-22 22:51:50, Wei Yang wrote: > > On Mon, Mar 14, 2022 at 10:54:03AM +0100, Michal Hocko wrote: > > >On Sat 12-03-22 07:16:22, Wei Yang wrote: > > >> There is no tree operation if mz is not on-tree. > > > > > >This doesn't explain problem you are trying to solve nor does it make > > >much sense to me TBH. > > > > > > > This just tries to make the code looks consistent. > > I guess this is rather subjective. One could argue that the check is > more descriptive because it obviously removes the node from the tree > when it is on the tree. Hmm... maybe yes. If someone else prefer the original one, I am ok with it. > -- > Michal Hocko > SUSE Labs
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index d70bf5cf04eb..344a7e891bc5 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -545,9 +545,11 @@ static void mem_cgroup_update_tree(struct mem_cgroup *memcg, int nid) unsigned long flags; spin_lock_irqsave(&mctz->lock, flags); - /* if on-tree, remove it */ - if (mz->on_tree) - __mem_cgroup_remove_exceeded(mz, mctz); + /* + * remove it first + * If not on-tree, no tree ops. + */ + __mem_cgroup_remove_exceeded(mz, mctz); /* * Insert again. mz->usage_in_excess will be updated. * If excess is 0, no tree ops.
There is no tree operation if mz is not on-tree. Let's remove the extra check. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- mm/memcontrol.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)