Re: [PATCH 1/1] rcu/tree: add emergency pool for headless case

[Uladzislau Rezki <urezki@gmail.com>]

Hello, Matthew.

> On Fri, Apr 03, 2020 at 07:30:51PM +0200, Uladzislau Rezki (Sony) wrote:
> > @@ -2877,6 +2885,12 @@ struct kfree_rcu_cpu {
> >  	bool initialized;
> >  	// Number of objects for which GP not started
> >  	int count;
> > +
> > +	/*
> > +	 * Reserved emergency pool for headless variant.
> > +	 */
> > +	int nr_emergency;
> > +	void **emergency;
> 
> This is a pretty expensive way to maintain an emergency pool.
> 
Well. I do not see what is expansive there, really. But i see
some drawbacks i would like to fix. First of all get rid of

<snip>
    krcp->emergency = kmalloc_array(rcu_nr_emergency_objs,
        sizeof(void *), GFP_NOWAIT);
<snip>

and second one, as you pointed below to use list instead of
an array. There is some advantages, first is no need in array
bound check, second, in case of list we can dynamically control
its length via exposed sysfs attribute.

> Try something like this ...
> 
> struct emergency_pool_object {
> 	union {
> 		struct whatever foo;
> 		struct {
> 			int remaining;
> 			void *next;
> 		};
> 	};
> };
> 
> struct kfree_rcu_cpu {
> 	...
> 	struct emergency_pool_object *epo;
> };
> 
> struct whatever *get_emergency_object(struct kfree_rcu_cpu *krc)
> {
> 	struct emergency_pool_object *epo = krc->epo;
> 	if (epo)
> 		krc->epo = epo->next;
> 	return &epo->foo;
> }
> 
> void alloc_emergency_objects(struct kfree_rcu_cpu *krc, int n)
> {
> 	int i = 0;
> 
> 	if (krc->epo)
> 		i = krc->epo->remaining;
> 
> 	while (++i < n) {
> 		struct emergency_pool_object *epo = kmalloc(sizeof(epo), GFP);
> 		epo->remaining = i;
> 		epo->next = krc->epo;
> 		krc->epo = epo;
> 	}
> }
> 
I will upload v2. I just stash objects in the list. Something like:

<snip>
@@ -2877,6 +2888,18 @@ struct kfree_rcu_cpu {
        bool initialized;
        // Number of objects for which GP not started
        int count;
+
+       /*
+        * Reserved emergency objects for headless variant.
+        * The objects are queued into the lock-less list,
+        * the length of the list is limited therefore we
+        * also have a counter.
+        *
+        * Actually we have the room for embedding a counter
+        * into our cached object, but let's keep it simple.
+        */
+       int nr_objs_elist;
+       struct llist_head elist;
 };
...
+static inline unsigned long *
+get_emergency_object(struct kfree_rcu_cpu *krcp)
+{
+       if (!krcp->nr_objs_elist)
+               return NULL;
+
+       krcp->nr_objs_elist--;
+       return (unsigned long *)
+               llist_del_first(&krcp->elist);
+}
+
+static inline bool
+put_emergency_object(struct kfree_rcu_cpu *krcp,
+       unsigned long *obj)
+{
+       /* Check the limit. */
+       if (krcp->nr_objs_elist >= rcu_nr_emergency_objs)
+               return false;
+
+       llist_add((struct llist_node *) obj, &krcp->elist);
+       krcp->nr_objs_elist++;
+       return true;
+}
<snip>

Thanks for your comments!

--
Vlad Rezki