Date:   Mon, 4 May 2020 08:24:37 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Uladzislau Rezki <urezki@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Theodore Y . Ts'o" <tytso@....edu>,
        Matthew Wilcox <willy@...radead.org>,
        Joel Fernandes <joel@...lfernandes.org>,
        RCU <rcu@...r.kernel.org>,
        Oleksiy Avramchenko <oleksiy.avramchenko@...ymobile.com>
Subject: Re: [PATCH 09/24] rcu/tree: cache specified number of objects

On Mon, May 04, 2020 at 02:43:23PM +0200, Uladzislau Rezki wrote:
> On Fri, May 01, 2020 at 02:27:49PM -0700, Paul E. McKenney wrote:
> > On Tue, Apr 28, 2020 at 10:58:48PM +0200, Uladzislau Rezki (Sony) wrote:
> > > Cache some extra objects per-CPU. During reclaim process
> > > some pages are cached instead of releasing by linking them
> > > into the list. Such approach provides O(1) access time to
> > > the cache.
> > > 
> > > That reduces number of requests to the page allocator, also
> > > that makes it more helpful if a low memory condition occurs.
> > > 
> > > A parameter reflecting the minimum allowed pages to be
> > > cached per one CPU is propagated via sysfs, it is read
> > > only, the name is "rcu_min_cached_objs".
> > > 
> > > Signed-off-by: Uladzislau Rezki (Sony) <urezki@...il.com>
> > > ---
> > >  kernel/rcu/tree.c | 64 ++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 60 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 89e9ca3f4e3e..d8975819b1c9 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -178,6 +178,14 @@ module_param(gp_init_delay, int, 0444);
> > >  static int gp_cleanup_delay;
> > >  module_param(gp_cleanup_delay, int, 0444);
> > >  
> > > +/*
> > > + * This rcu parameter is read-only, but can be write also.
> > 
> > You mean that although the parameter is read-only, you see no reason
> > why it could not be converted to writeable?
> > 
> I added just a note. If it is writable, then we can change the size of the
> per-CPU cache dynamically, i.e. "echo 5 > /sys/.../rcu_min_cached_objs"
> would cache 5 pages. But i do not have a strong opinion if it should be
> writable.
> 
> > If it was writeable, and a given CPU had the maximum numbr of cached
> > objects, the rcu_min_cached_objs value was decreased, but that CPU never
> > saw another kfree_rcu(), would the number of cached objects change?
> > 
> No. It works the way: unqueue the page from cache in the kfree_rcu(),
> whereas "rcu work" will put it back if number of objects < rcu_min_cached_objs,
> if >= will free the page.

Just to make sure I understand...  If someone writes a smaller number to
the sysfs variable, the per-CPU caches will be decreased at that point,
immediately during that sysfs write?  Or are you saying something else?

> > (Just curious, not asking for a change in functionality.)
> > 
> > > + * It reflects the minimum allowed number of objects which
> > > + * can be cached per-CPU. Object size is equal to one page.
> > > + */
> > > +int rcu_min_cached_objs = 2;
> > > +module_param(rcu_min_cached_objs, int, 0444);
> > > +
> > >  /* Retrieve RCU kthreads priority for rcutorture */
> > >  int rcu_get_gp_kthreads_prio(void)
> > >  {
> > > @@ -2887,7 +2895,6 @@ struct kfree_rcu_cpu_work {
> > >   * struct kfree_rcu_cpu - batch up kfree_rcu() requests for RCU grace period
> > >   * @head: List of kfree_rcu() objects not yet waiting for a grace period
> > >   * @bhead: Bulk-List of kfree_rcu() objects not yet waiting for a grace period
> > > - * @bcached: Keeps at most one object for later reuse when build chain blocks
> > >   * @krw_arr: Array of batches of kfree_rcu() objects waiting for a grace period
> > >   * @lock: Synchronize access to this structure
> > >   * @monitor_work: Promote @head to @head_free after KFREE_DRAIN_JIFFIES
> > > @@ -2902,7 +2909,6 @@ struct kfree_rcu_cpu_work {
> > >  struct kfree_rcu_cpu {
> > >  	struct rcu_head *head;
> > >  	struct kfree_rcu_bulk_data *bhead;
> > > -	struct kfree_rcu_bulk_data *bcached;
> > >  	struct kfree_rcu_cpu_work krw_arr[KFREE_N_BATCHES];
> > >  	raw_spinlock_t lock;
> > >  	struct delayed_work monitor_work;
> > > @@ -2910,6 +2916,15 @@ struct kfree_rcu_cpu {
> > >  	bool initialized;
> > >  	// Number of objects for which GP not started
> > >  	int count;
> > > +
> > > +	/*
> > > +	 * Number of cached objects which are queued into
> > > +	 * the lock-less list. This cache is used by the
> > > +	 * kvfree_call_rcu() function and as of now its
> > > +	 * size is static.
> > > +	 */
> > > +	struct llist_head bkvcache;
> > > +	int nr_bkv_objs;
> > >  };
> > >  
> > >  static DEFINE_PER_CPU(struct kfree_rcu_cpu, krc) = {
> > > @@ -2946,6 +2961,31 @@ krc_this_cpu_unlock(struct kfree_rcu_cpu *krcp, unsigned long flags)
> > >  	local_irq_restore(flags);
> > >  }
> > >  
> > > +static inline struct kfree_rcu_bulk_data *
> > > +get_cached_bnode(struct kfree_rcu_cpu *krcp)
> > > +{
> > > +	if (!krcp->nr_bkv_objs)
> > > +		return NULL;
> > > +
> > > +	krcp->nr_bkv_objs--;
> > > +	return (struct kfree_rcu_bulk_data *)
> > > +		llist_del_first(&krcp->bkvcache);
> > > +}
> > > +
> > > +static inline bool
> > > +put_cached_bnode(struct kfree_rcu_cpu *krcp,
> > > +	struct kfree_rcu_bulk_data *bnode)
> > > +{
> > > +	/* Check the limit. */
> > > +	if (krcp->nr_bkv_objs >= rcu_min_cached_objs)
> > > +		return false;
> > > +
> > > +	llist_add((struct llist_node *) bnode, &krcp->bkvcache);
> > > +	krcp->nr_bkv_objs++;
> > > +	return true;
> > > +
> > > +}
> > > +
> > >  /*
> > >   * This function is invoked in workqueue context after a grace period.
> > >   * It frees all the objects queued on ->bhead_free or ->head_free.
> > > @@ -2981,7 +3021,12 @@ static void kfree_rcu_work(struct work_struct *work)
> > >  		kfree_bulk(bhead->nr_records, bhead->records);
> > >  		rcu_lock_release(&rcu_callback_map);
> > >  
> > > -		if (cmpxchg(&krcp->bcached, NULL, bhead))
> > > +		krcp = krc_this_cpu_lock(&flags);
> > 
> > Presumably the list can also be accessed without holding this lock,
> > because otherwise we shouldn't need llist...
> > 
> Hm... We increase the number of elements in cache, therefore it is not
> lockless. From the other hand i used llist_head to maintain the cache
> because it is single linked list, we do not need "*prev" link. Also
> we do not need to init the list.
> 
> But i can change it to list_head. Please let me know if i need :)

Hmmm...  Maybe it is time for a non-atomic singly linked list?  In the RCU
callback processing, the operations were open-coded, but they have been
pushed into include/linux/rcu_segcblist.h and kernel/rcu/rcu_segcblist.*.

Maybe some non-atomic/protected/whatever macros in the llist.h file?
Or maybe just open-code the singly linked list?  (Probably not the
best choice, though.)  Add comments stating that the atomic properties
of the llist functions aren't neded?  Something else?

The comments would be a good start.  Just to take pity on people seeing
the potential for concurrency and wondering how the concurrent accesses
actually happen.  ;-)

							Thanx, Paul

Powered by blists - more mailing lists