Re: [PATCH 2/2] kfree_rcu() should use kfree_bulk() interface

[Rao Shoaib]

On 04/03/2018 01:58 PM, Matthew Wilcox wrote:
> On Tue, Apr 03, 2018 at 10:22:53AM -0700, rao.shoaib@xxxxxxxxxx wrote:
> > +++ b/mm/slab.h
> > @@ -80,6 +80,29 @@ extern const struct kmalloc_info_struct {
> >   	unsigned long size;
> >   } kmalloc_info[];
> >   
> > +#define	RCU_MAX_ACCUMULATE_SIZE	25
> > +
> > +struct rcu_bulk_free_container {
> > +	struct	rcu_head rbfc_rcu;
> > +	int	rbfc_entries;
> > +	void	*rbfc_data[RCU_MAX_ACCUMULATE_SIZE];
> > +	struct	rcu_bulk_free *rbfc_rbf;
> > +};
> > +
> > +struct rcu_bulk_free {
> > +	struct	rcu_head rbf_rcu; /* used to schedule monitor process */
> > +	spinlock_t	rbf_lock;
> > +	struct		rcu_bulk_free_container *rbf_container;
> > +	struct		rcu_bulk_free_container *rbf_cached_container;
> > +	struct		rcu_head *rbf_list_head;
> > +	int		rbf_list_size;
> > +	int		rbf_cpu;
> > +	int		rbf_empty;
> > +	int		rbf_polled;
> > +	bool		rbf_init;
> > +	bool		rbf_monitor;
> > +};
> I think you might be better off with an IDR.  The IDR can always
> contain one entry, so there's no need for this 'rbf_list_head' or
> __rcu_bulk_schedule_list.  The IDR contains its first 64 entries in
> an array (if that array can be allocated), so it's compatible with the
> kfree_bulk() interface.
I have just familiarized myself with what IDR is by reading your 
article. If I am incorrect please correct me.

The list and head you have pointed are only used if the container can 
not be allocated. That could happen with IDR as well. Note that the 
containers are allocated at boot time and are re-used.

IDR seems to have some overhead, such as I have to specifically add the 
pointer and free the ID, plus radix tree maintenance.

The change would also require retesting. So I would like to keep the 
current design.

Regards,

Shoaib