Re: [PATCH] ifb: add multi-queue support

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

Changli Gao a écrit :
> ifb: add multi-queue support
> 
> Add multi-queue support, and one kernel thread is created for per queue.
> It can used to emulate multi-queue NIC in software, and distribute work
> among CPUs.
> gentux linux # modprobe ifb numtxqs=2
> gentux linux # ifconfig ifb0 up
> gentux linux # pgrep ifb0
> 18508
> 18509
> gentux linux # taskset -p 1 18508
> pid 18508's current affinity mask: 3
> pid 18508's new affinity mask: 1
> gentux linux # taskset -p 2 18509
> pid 18509's current affinity mask: 3
> pid 18509's new affinity mask: 2
> gentux linux # tc qdisc add dev br0 ingress
> gentux linux # tc filter add dev br0 parent ffff: protocol ip basic
> action mirred egress redirect dev ifb0
> 
> This patch also introduces a ip link option "numtxqs" for specifying the
> number of the TX queues. so you can add a new ifb with the command:
> 
> ip link add numtxqs 4 type ifb
> 
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
> drivers/net/ifb.c | 414 ++++++++++++++++++++++++++++++++----------------
> include/linux/if_link.h | 1
> net/core/rtnetlink.c | 3
> 3 files changed, 283 insertions(+), 135 deletions(-)
> 
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index 69c2566..ac04e85 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -33,161 +33,157 @@
>  #include <linux/etherdevice.h>
>  #include <linux/init.h>
>  #include <linux/moduleparam.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +#include <linux/kthread.h>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <net/ip.h>
>  #include <net/pkt_sched.h>
>  #include <net/net_namespace.h>
>  
> -#define TX_TIMEOUT  (2*HZ)
> -
>  #define TX_Q_LIMIT    32
> +
> +struct ifb_private_q {
> +	struct net_device	*dev;
> +	struct sk_buff_head	rq;
> +	struct sk_buff_head	tq;
> +	wait_queue_head_t	wq;
> +	struct task_struct	*task;
> +	unsigned long		rx_packets;
> +	unsigned long		rx_bytes;
> +	unsigned long		rx_dropped;
> +} ____cacheline_aligned_in_smp;
> +

Could you split this struct in two parts, one used by ifb_xmit(),
one used by the ifb_thread() ?
This would reduce number of cache line ping pongs...


struct ifb_private_q {

	struct net_device	*dev;
	struct task_struct	*task;

/* used by ifb_xmit() */

	struct sk_buff_head	rq;
	unsigned long		rx_packets;
	unsigned long		rx_bytes;
	unsigned long		rx_dropped;
	wait_queue_head_t	wq;

/* used by ifb_thread() */

	struct sk_buff_head	tq ____cacheline_aligned_in_smp;
} ____cacheline_aligned_in_smp;


Or even better, tq could be local to ifb_xmit()

ifb_xmit() would purge it itself, and could use __skb_dequeue() and
other lockless primitives.

The whole :

while ((skb = skb_dequeue(&pq->rq)) != NULL)
	skb_queue_tail(&pq->tq, skb);

could be optimized a lot IMHO, its almost a skb_queue_splice() thing...