Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2374bbca7651b671ec934fa5c630cbe3eed3b496.camel@redhat.com>
Subject: Re: [PATCH 2/2] mm/page_alloc: Add remote draining support to
 per-cpu lists
From:   Nicolas Saenz Julienne <nsaenzju@redhat.com>
To:     Marcelo Tosatti <mtosatti@redhat.com>
Cc:     akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, frederic@kernel.org, tglx@linutronix.de,
        mgorman@suse.de, linux-rt-users@vger.kernel.org, vbabka@suse.cz,
        cl@linux.com, paulmck@kernel.org, willy@infradead.org
Date:   Tue, 15 Feb 2022 09:47:35 +0100
In-Reply-To: <YgKQfsznPUDN34un@fuller.cnet>
References: <20220208100750.1189808-1-nsaenzju@redhat.com>
         <20220208100750.1189808-3-nsaenzju@redhat.com>
         <YgKQfsznPUDN34un@fuller.cnet>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.3 (3.42.3-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Tue, 2022-02-08 at 12:47 -0300, Marcelo Tosatti wrote:
> > Changes since RFC:
> >  - Avoid unnecessary spin_lock_irqsave/restore() in free_pcppages_bulk()
> >  - Add more detail to commit and code comments.
> >  - Use synchronize_rcu() instead of synchronize_rcu_expedited(), the RCU
> >    documentation says to avoid it unless really justified. I don't think
> >    it's justified here, if we can schedule and join works, waiting for
> >    an RCU grace period is OK.
> 
> https://patchwork.ozlabs.org/project/netdev/patch/1306228052.3026.16.camel@edumazet-laptop/
> 
> Adding 100ms to direct reclaim path might be problematic. It will also
> slowdown kcompactd (note it'll call drain_all_pages for each zone).

I did some measurements on an idle machine, worst case was ~30ms. I agree that
might too much for direct reclaim, so I'll switch back to expedited and add a
comment.

> >  - Avoid sparse warnings by using rcu_access_pointer() and
> >    rcu_dereference_protected().
> > 
> >  include/linux/mmzone.h |  22 +++++-
> >  mm/page_alloc.c        | 155 ++++++++++++++++++++++++++---------------
> >  mm/vmstat.c            |   6 +-
> >  3 files changed, 120 insertions(+), 63 deletions(-)
> > 
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index b4cb85d9c6e8..b0b593fd8e48 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -388,13 +388,31 @@ struct per_cpu_pages {
> >  	short expire;		/* When 0, remote pagesets are drained */
> >  #endif
> >  
> > -	struct pcplists *lp;
> > +	/*
> > +	 * As a rule of thumb, any access to struct per_cpu_pages's 'lp' has
> > +	 * happen with the pagesets local_lock held and using
> > +	 * rcu_dereference_check(). If there is a need to modify both
> > +	 * 'lp->count' and 'lp->lists' in the same critical section 'pcp->lp'
> > +	 * can only be derefrenced once. See for example:
> 
> Typo.

Noted.

Thanks!

-- 
Nicolás Sáenz