Re: [patch 2/7] CAN: Add PF_CAN core module

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Fri, May 18, 2007 at 11:19:01AM +0200, Oliver Hartkopp wrote:
> Hi Urs, Hello Paul,
> 
> i assume Paul refers to the can_rx_delete_all() function that adds each 
> receive list entry for rcu removal using the can_rx_delete RCU callback, 
> right?
> 
> So the idea would be to create a second RCU callback - e.g. 
> can_rx_delete_list() - that removes the complete list inside the RCU 
> callback?!?
> The list removal would therefore be processed inside this new 
> can_rx_delete_list() in RCU context and not inside can_rx_delete_all().
> 
> @Paul: Was this your intention?

My intention was that the list-removing be placed into can_rcv_lists_delete(),
perhaps as follows:

static void can_rx_delete_all(struct hlist_head *rl)
{
	struct receiver *r;
	struct hlist_node *n;

	hlist_for_each_entry(r, n, rl, list) {
		hlist_del(&r->list);
		kmem_cache_free(rcv_cache, r);
	}
}

static void can_rcv_lists_delete(struct rcu_head *rp)
{
	struct dev_rcv_lists *d = container_of(rp, struct dev_rcv_lists, rcu);

	/* remove all receivers hooked at this netdevice */
	can_rx_delete_all(&d->rx_err);
	can_rx_delete_all(&d->rx_all);
	can_rx_delete_all(&d->rx_fil);
	can_rx_delete_all(&d->rx_inv);
	can_rx_delete_all(&d->rx_eff);
	for (i = 0; i < 2048; i++)
		can_rx_delete_all(&d->rx_sff[i]);
	kfree(d);
}

Then the code in can_notifier() can reduce to the following:

	if (d) {
		hlist_del_rcu(&d->list);

		/* used to be a string of can_rx_delete_all(). */
	} else
		printk(KERN_ERR "can: notifier: receive list not "
		       "found for dev %s\n", dev->name);

	spin_lock_bh(&rcv_lists_lock);

	if (d) {
		call_rcu(&d->rcu, can_rcv_lists_delete);
	}

This moves the traversal work into the callback function.  This is not
a problem for CONFIG_PREEMPT_RT and non-CONFIG_PREEMPT, but not sure
about CONFIG_PREEMPT.

But it sure has the potential to cut down on a bunch of call_rcu()
work...

						Thanx, Paul

> Best regards,
> Oliver
> 
> Paul E. McKenney wrote:
> >On Wed, May 16, 2007 at 04:51:02PM +0200, Urs Thuermann wrote:
> >  
> >>This patch adds the CAN core functionality but no protocols or drivers.
> >>No protocol implementations are included here.  They come as separate
> >>patches.  Protocol numbers are already in include/linux/can.h.
> >>    
> >
> >Interesting!  One question called out below -- why do call_rcu() on each
> >piece of the struct dev_rcv_lists, instead of doing call_rcu() on the
> >whole thing and having the RCU callback free up the pieces?  Given that
> >all the pieces are call_rcu()ed separately, there had better not be
> >persistent pointers to the pieces, right?
> >
> >Doing it in one chunk would make the code a bit simpler and also reduce
> >the RCU overhead a bit.
> >
> >Or am I missing something subtle here?
> >
> >						Thanx, Paul