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

[Eric Dumazet <eric.dumazet@gmail.com>]

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.

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.

> 
> ...
>> +/*
>> + * 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... :)

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.

Thanks