Messages in this thread Patch in this message | | | Date | Thu, 31 Mar 2022 18:11:26 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH v2] mm/list_lru: Fix possible race in memcg_reparent_list_lru_node() |
| |
On Thu, 31 Mar 2022 09:46:52 +0200 Michal Hocko <mhocko@suse.com> wrote:
> On Thu 31-03-22 06:39:56, Shakeel Butt wrote: > > On Wed, Mar 30, 2022 at 07:48:45PM -0700, Roman Gushchin wrote: > > > > > > > > [...] > > > > > > > > > But honestly, I’d drop the original optimization together with > > > the fix, if only there is no _real world_ data on the problem and > > > the improvement. It seems like it has started as a nice simple > > > improvement, but the race makes it complex and probably not worth > > > the added complexity and fragility. > > > > I agree with dropping the original optimization as it is not really > > fixing an observed issue which may justify adding some complexity. > > Completely agreed. The patch as it is proposed is not really acceptable > IMHO and I have to say I am worried that this is not the first time we > are in a situation when a follow up fixes or unrelated patches are > growing in complexity to fit on top of a performance optimizations which > do not refer to any actual numbers.
Yup. I did this:
From: Andrew Morton <akpm@linux-foundation.org> Subject: mm/list_lru.c: revert "mm/list_lru: optimize memcg_reparent_list_lru_node()"
405cc51fc1049c73 ("mm/list_lru: optimize memcg_reparent_list_lru_node()") has subtle races which are proving ugly to fix. Revert the original optimization. If quantitative testing indicates that we have a significant problem here then other implementations can be looked at.
Fixes: 405cc51fc1049c73 ("mm/list_lru: optimize memcg_reparent_list_lru_node()") Cc: Waiman Long <longman@redhat.com> Cc: Roman Gushchin <roman.gushchin@linux.dev> Cc: Muchun Song <songmuchun@bytedance.com> Cc: Michal Hocko <mhocko@suse.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Shakeel Butt <shakeelb@google.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> ---
mm/list_lru.c | 6 ------ 1 file changed, 6 deletions(-)
--- a/mm/list_lru.c~revert-1 +++ a/mm/list_lru.c @@ -395,12 +395,6 @@ static void memcg_reparent_list_lru_node struct list_lru_one *src, *dst; /* - * If there is no lru entry in this nlru, we can skip it immediately. - */ - if (!READ_ONCE(nlru->nr_items)) - return; - - /* * Since list_lru_{add,del} may be called under an IRQ-safe lock, * we have to use IRQ-safe primitives here to avoid deadlock. */ _
| |