Re: [PATCH net-next-2.6] net: Xmit Packet Steering (XPS)

[Jarek Poplawski <jarkao2@gmail.com>]

On Fri, Nov 20, 2009 at 03:45:42PM +0100, Eric Dumazet wrote:
> Jarek Poplawski a écrit :
> > On 20-11-2009 00:46, Eric Dumazet wrote:
> >> Here is first version of XPS.
> >>
> >> Goal of XPS is to free TX completed skbs by the cpu that submitted the transmit.
> > 
> > But why?... OK, you write in another message about sock_wfree(). Then
> > how about users, who don't sock_wfree (routers)? Will there be any way
> > to disable it?
> 
> 
> This is open for discussion, but I saw no problem with routing workloads.

IMHO, it should depend on testing: if you can  prove there is a
distinct gain in "common" use case (which isn't probably a numa box
used e.g. for google.com or even kernel.org yet), and no visible
slowdown for such a router, then we could probably forget about
disabling, and look more at optimizations of the fast path.

> 
> sock_wfree() is not that expensive for tcp anyway.
> You also have a cost of kfreeing() two blocks of memory per skb, if allocation was done by another cpu.
> 
> If this happens to be a problem, we can immediately free packet if it 
> has no destructors :
> 
> At xmit time, initialize skb->sending_cpu like that
> 
> skb->sending_cpu = (skb->destructor) ? smp_processor_id() : 0xFFFF;
> 
> to make sure we dont touch too many cache lines at tx completion time.
> 
> 
> >> +/*
> >> + * XPS : Xmit Packet Steering
> >> + *
> >> + * TX completion packet freeing is performed on cpu that sent packet.
> >> + */
> >> +#if defined(CONFIG_SMP)
> > 
> > Shouldn't it be in the Makefile?
> 
> It is in Makefile too, I let it in prelim code to make it clear this was CONFIG_SMP only.

Aha! Now it's clear why I made this mistake. ;-)
> 
> > 
> > ...
> >> +/*
> >> + * called at end of net_rx_action()
> >> + * preemption (and cpu migration/offline/online) disabled
> >> + */
> >> +void xps_flush(void)
> >> +{
> >> +	int cpu, prevlen;
> >> +	struct sk_buff_head *head = per_cpu_ptr(xps_array, smp_processor_id());
> >> +	struct xps_pcpu_queue *q;
> >> +	struct sk_buff *skb;
> >> +
> >> +	for_each_cpu_mask_nr(cpu, __get_cpu_var(xps_cpus)) {
> >> +		q = &per_cpu(xps_pcpu_queue, cpu);
> >> +		if (cpu_online(cpu)) {
> >> +			spin_lock(&q->list.lock);
> > 
> > This lock probably needs irq disabling: let's say 2 cpus run this at
> > the same time and both are interrupted with these (previously
> > scheduled) IPIs?
> 
> Repeat after me :
> 
> lockdep is my friend, lockdep is my friend, lockdep is my friend... :)

Hmm... Actually, why did I have to do lockdep's job...
> 
> Seriously, I must think again on this locking schem.
> 
> >> +static void remote_free_skb_list(void *arg)
> >> +{
> >> +	struct sk_buff *last;
> >> +	struct softnet_data *sd;
> >> +	struct xps_pcpu_queue *q = arg; /* &__get_cpu_var(xps_pcpu_queue); */
> >> +
> >> +	spin_lock(&q->list.lock);
> >> +
> >> +	last = q->list.prev;
> > 
> > Is q->list handled in case this cpu goes down before this IPI is
> > triggered?
> 
> 
> [block migration] (how ? this is the question)
> 
> if (cpu_online(cpu)) { 
> 	give_work_to_cpu(cpu);
> 	trigger IPI
> } else {
> 	handle_work_ourself()
> }
> 
> [unblock migration]
> 
> General problem is : what guards cpu going off line between the if (cpu_online(cpu))
> and the IPI.
> I dont know yet, but it seems that disabling preemption is enough to get this
> guarantee. This seems strange.
> 
> We can add a notifier (or better call a function from existing one : dev_cpu_callback()) to 
> flush this queue when necessary.

Using dev_cpu_callback() looks quite obvious to me.

Jarek P.