netpoll: allow execution of multiple rx_hooks per interface

PATCH Applied Daniel Borkmann 4 participants 13 messages Jan 6, 2010 – Jan 14, 2010

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Daniel Borkmann

author

2010-01-06 20:54 UTC #0 lore raw

PATCH 1/1 netpoll: allow execution of multiple rx_hooks per interface

Hi,

this patch allows the registration and _execution_ of multiple netpoll rx_hooks per interface. Currently, it is possible to register multiple netpoll structures to one interface, _but_ only one single rx_hook (from the netpoll struct that has been registered last) can be executed, which was an oversight in the implementation [1].
So, this patch fixes it. I've sucessfully tested it within 2.6.32.2 with the registration of multiple rx_hook clients for several times. I'd appreciate comments / feedback.

Thanks,
Daniel

[1] http://lwn.net/Articles/140852/

Signed-off-by Daniel Borkmann <[email protected]>

include/linux/netpoll.h +1 -0

diff -Nur a/include/linux/netpoll.h b/include/linux/netpoll.h
@@ -21,6 +21,7 @@
 	__be32 local_ip, remote_ip;
 	u16 local_port, remote_port;
 	u8 remote_mac[ETH_ALEN];
+	struct netpoll *next; /* rx list for net device */
 };
 
 struct netpoll_info {

net/core/netpoll.c +74 ~~-18~~

148 lines not on this page · raw message

Matt Mackall

2010-01-07 03:54 UTC #1 lore raw

Quoted 10 lines · Daniel Borkmann

Hi,
this patch allows the registration and _execution_ of multiple netpoll rx_hooks per interface. Currently, it is possible to register multiple netpoll structures to one interface, _but_ only one single rx_hook (from the netpoll struct that has been registered last) can be executed, which was an oversight in the implementation [1].
So, this patch fixes it. I've sucessfully tested it within 2.6.32.2 with the registration of multiple rx_hook clients for several times. I'd appreciate comments / feedback.

(grumbles about cc:)

Please inline patches so they can be reviewed easily in reply.

- struct netpoll *np = npi->rx_np;
+ struct netpoll **np = &npi->rx_np;

- if (!np)
+ if (!(*np))

This makes everything horrible. Can you avoid the double indirection? Using a list head might be a good answer.

Signature · 1 line

http://selenic.com : development and support for Mercurial and Linux

David Miller

maintainer

2010-01-07 09:01 UTC #2 lore raw

From: Matt Mackall <[email protected]>
Date: Wed, 06 Jan 2010 21:54:05 -0600

Quoted 9 lines

Please inline patches so they can be reviewed easily in reply.

net/core/netpoll.c +2 -2

-       struct netpoll *np = npi->rx_np;
+       struct netpoll **np = &npi->rx_np;
 
-       if (!np)
+       if (!(*np))

Quoted 3 lines

This makes everything horrible. Can you avoid the double indirection? Using a list head might be a good answer.

Agreed on all counts.

Daniel Borkmann

author

2010-01-07 19:06 UTC #3 lore raw

Quoted 9 lines · David Miller

From: Matt Mackall <[email protected]>
Date: Wed, 06 Jan 2010 21:54:05 -0600
Quoted 6 lines
Please inline patches so they can be reviewed easily in reply.
net/core/netpoll.c +1 -1
-       struct netpoll *np = npi->rx_np;
+       struct netpoll **np = &npi->rx_np;

Quoted 9 lines

Quoted 6 lines
net/core/netpoll.c +1 -1
-       if (!np)
+       if (!(*np))
This makes everything horrible. Can you avoid the double indirection? Using a list head might be a good answer.
Agreed on all counts.

Agreed on the double indirection, I'll fix it.

I've already considered the list_head structure, but then I was the opinion that a double linked list might not be necessary for this, so I did it that way ... (compare: kernel notifier by Alan Cox). If you insist on that I'll fix it of course ;)

Best regards,
Daniel

P.s.: Sorry Matt for not CCing. I mainly took those addresses from Jeffs post.

Daniel Borkmann

author

2010-01-08 00:20 UTC #4 lore raw

Quoted 19 lines · Daniel Borkmann

Quoted 9 lines · David Miller
From: Matt Mackall <[email protected]>
Date: Wed, 06 Jan 2010 21:54:05 -0600
Quoted 6 lines
Please inline patches so they can be reviewed easily in reply.
(quoted diff) +1 -1
-       struct netpoll *np = npi->rx_np;
+       struct netpoll **np = &npi->rx_np;
Quoted 8 lines
Quoted 6 lines
(quoted diff) +1 -1
-       if (!np)
+       if (!(*np))
This makes everything horrible. Can you avoid the double indirection? Using a list head might be a good answer.
Agreed on all counts.

Quoted 6 lines

Agreed on the double indirection, I'll fix it.
I've already considered the list_head structure, but then I was the opinion that a double linked list might not be necessary for this, so I did it that way ... (compare: kernel notifier by Alan Cox). If you insist on that I'll fix it of course ;)

So, here's the list head implementation. Tested on both of my machines with several rx_hook clients.

Best regards,
Daniel

Signed-off-by Daniel Borkmann <[email protected]>

include/linux/netpoll.h +8 -3

diff -Nur a/include/linux/netpoll.h b/include/linux/netpoll.h
@@ -21,15 +21,20 @@
 	__be32 local_ip, remote_ip;
 	u16 local_port, remote_port;
 	u8 remote_mac[ETH_ALEN];
+
+	struct list_head rx; /* rx_np list element */
 };
 
 struct netpoll_info {
 	atomic_t refcnt;
+
 	int rx_flags;
 	spinlock_t rx_lock;
-	struct netpoll *rx_np; /* netpoll that registered an rx_hook */
+	struct list_head rx_np; /* netpolls that registered an rx_hook */
+
 	struct sk_buff_head arp_tx; /* list of arp requests to reply to */
 	struct sk_buff_head txq;
+
 	struct delayed_work tx_work;
 };

@@ -51,7 +56,7 @@
 	unsigned long flags;
 	int ret = 0;
 
-	if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
+	if (!npinfo || (list_empty(&npinfo->rx_np) && !npinfo->rx_flags))
 		return 0;
 
 	spin_lock_irqsave(&npinfo->rx_lock, flags);

@@ -67,7 +72,7 @@
 {
 	struct netpoll_info *npinfo = skb->dev->npinfo;
 
-	return npinfo && (npinfo->rx_np || npinfo->rx_flags);
+	return npinfo && (!list_empty(&npinfo->rx_np) || npinfo->rx_flags);
 }
 
 static inline int netpoll_receive_skb(struct sk_buff *skb)

net/core/netpoll.c +122 ~~-94~~

284 lines not on this page · raw message

Matt Mackall

Acked-by

2010-01-11 23:21 UTC #5 lore raw

Quoted 22 lines · Daniel Borkmann

Quoted 21 lines · Daniel Borkmann
Quoted 9 lines · David Miller
From: Matt Mackall <[email protected]>
Date: Wed, 06 Jan 2010 21:54:05 -0600
Quoted 6 lines
Please inline patches so they can be reviewed easily in reply.
(quoted diff) +1 -1
-       struct netpoll *np = npi->rx_np;
+       struct netpoll **np = &npi->rx_np;
Quoted 8 lines
Quoted 6 lines
(quoted diff) +1 -1
-       if (!np)
+       if (!(*np))
This makes everything horrible. Can you avoid the double indirection? Using a list head might be a good answer.
Agreed on all counts.
Agreed on the double indirection, I'll fix it.

Quoted 7 lines

Quoted 4 lines
I've already considered the list_head structure, but then I was the opinion that a double linked list might not be necessary for this, so I did it that way ... (compare: kernel notifier by Alan Cox). If you insist on that I'll fix it of course ;)
So, here's the list head implementation. Tested on both of my machines with several rx_hook clients.

Looks pretty good. Dave?

Acked-by Matt Mackall <[email protected]>

Signature · 1 line

http://selenic.com : development and support for Mercurial and Linux

David Miller

maintainer

2010-01-11 23:59 UTC #6 lore raw

From: Matt Mackall <[email protected]>
Date: Mon, 11 Jan 2010 17:21:48 -0600

Quoted 3 lines

Looks pretty good. Dave?
Acked-by Matt Mackall <[email protected]>

I don't like the loop for RX ARP processing.

The packet contents aren't going to change, so doing basic packet validation inside of the "for each RX client" loop of arp_reply() doesn't make any sense.

Matt Mackall

2010-01-12 00:03 UTC #7 lore raw

Quoted 5 lines · David Miller

From: Matt Mackall <[email protected]>
Date: Mon, 11 Jan 2010 17:21:48 -0600
Quoted 2 lines
Looks pretty good. Dave?

Quoted 7 lines

Quoted 1 line
Acked-by Matt Mackall <[email protected]>
I don't like the loop for RX ARP processing.
The packet contents aren't going to change, so doing basic packet validation inside of the "for each RX client" loop of arp_reply() doesn't make any sense.

True. Dan, please help our poor compilers with some manual loop invariant motion.

Signature · 1 line

http://selenic.com : development and support for Mercurial and Linux

Daniel Borkmann

author

2010-01-12 00:09 UTC #8 lore raw

Quoted 7 lines · Matt Mackall

Quoted 6 lines · David Miller
From: Matt Mackall <[email protected]>
Date: Mon, 11 Jan 2010 17:21:48 -0600
Quoted 3 lines
Looks pretty good. Dave?
Acked-by Matt Mackall <[email protected]>

Quoted 8 lines

Quoted 5 lines
I don't like the loop for RX ARP processing.
The packet contents aren't going to change, so doing basic packet validation inside of the "for each RX client" loop of arp_reply() doesn't make any sense.
True. Dan, please help our poor compilers with some manual loop invariant motion.

Okay, true. I'll fix this by tomorrow and resend the patch.

Best regards,
Daniel

Daniel Borkmann

author

2010-01-13 00:27 UTC #9 lore raw

Quoted 10 lines · Daniel Borkmann

Quoted 9 lines · Matt Mackall
Quoted 8 lines · David Miller
From: Matt Mackall <[email protected]>
Date: Mon, 11 Jan 2010 17:21:48 -0600
Quoted 3 lines
Looks pretty good. Dave?
Acked-by Matt Mackall <[email protected]>
I don't like the loop for RX ARP processing.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
The packet contents aren't going to change, so doing basic packet validation inside of the "for each RX client" loop of arp_reply() doesn't make any sense.
True. Dan, please help our poor compilers with some manual loop invariant motion.
Okay, true. I'll fix this by tomorrow and resend the patch.

Here the fix of the RX ARP processing routine. Content that isn't going to change is out-of-loop.
Successfully tested on my machines.

Signed-off-by Daniel Borkmann <[email protected]>

include/linux/netpoll.h +8 -3

diff -Nur a/include/linux/netpoll.h b/include/linux/netpoll.h
@@ -21,15 +21,20 @@
 	__be32 local_ip, remote_ip;
 	u16 local_port, remote_port;
 	u8 remote_mac[ETH_ALEN];
+
+	struct list_head rx; /* rx_np list element */
 };
 
 struct netpoll_info {
 	atomic_t refcnt;
+
 	int rx_flags;
 	spinlock_t rx_lock;
-	struct netpoll *rx_np; /* netpoll that registered an rx_hook */
+	struct list_head rx_np; /* netpolls that registered an rx_hook */
+
 	struct sk_buff_head arp_tx; /* list of arp requests to reply to */
 	struct sk_buff_head txq;
+
 	struct delayed_work tx_work;
 };

@@ -51,7 +56,7 @@
 	unsigned long flags;
 	int ret = 0;
 
-	if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
+	if (!npinfo || (list_empty(&npinfo->rx_np) && !npinfo->rx_flags))
 		return 0;
 
 	spin_lock_irqsave(&npinfo->rx_lock, flags);

@@ -67,7 +72,7 @@
 {
 	struct netpoll_info *npinfo = skb->dev->npinfo;
 
-	return npinfo && (npinfo->rx_np || npinfo->rx_flags);
+	return npinfo && (!list_empty(&npinfo->rx_np) || npinfo->rx_flags);
 }
 
 static inline int netpoll_receive_skb(struct sk_buff *skb)

net/core/netpoll.c +109 ~~-66~~

252 lines not on this page · raw message

Jeff Moyer

2010-01-13 13:57 UTC #10 lore raw

Quoted 14 lines · Daniel Borkmann

Quoted 13 lines · Daniel Borkmann
Quoted 12 lines · Matt Mackall
Quoted 11 lines · David Miller
From: Matt Mackall <[email protected]>
Date: Mon, 11 Jan 2010 17:21:48 -0600
Quoted 3 lines
Looks pretty good. Dave?
Acked-by Matt Mackall <[email protected]>
I don't like the loop for RX ARP processing.
The packet contents aren't going to change, so doing basic packet validation inside of the "for each RX client" loop of arp_reply() doesn't make any sense.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
True. Dan, please help our poor compilers with some manual loop invariant motion.
Okay, true. I'll fix this by tomorrow and resend the patch.
Here the fix of the RX ARP processing routine. Content that isn't going to change is out-of-loop.
Successfully tested on my machines.

Against what tree does this patch apply? It doesn't apply to Linus's git tree. Also, in the future, could you use the -p option to diff so we can see what function or data structure is being modified? It really helps in reviewing.

Thanks!
Jeff

Daniel Borkmann

author

2010-01-13 16:53 UTC #11 lore raw

2010/1/13 Jeff Moyer <[email protected]>:

Quoted 21 lines

Quoted 19 lines · Daniel Borkmann
Quoted 17 lines · Daniel Borkmann
Quoted 6 lines · Matt Mackall
Quoted 5 lines · David Miller
From: Matt Mackall <[email protected]>
Date: Mon, 11 Jan 2010 17:21:48 -0600
Quoted 2 lines
Looks pretty good. Dave?
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Acked-by Matt Mackall <[email protected]>
I don't like the loop for RX ARP processing.
The packet contents aren't going to change, so doing basic packet validation inside of the "for each RX client" loop of arp_reply() doesn't make any sense.
True. Dan, please help our poor compilers with some manual loop invariant motion.
Okay, true. I'll fix this by tomorrow and resend the patch.

Quoted 8 lines

Quoted 3 lines
Here the fix of the RX ARP processing routine. Content that isn't going to change is out-of-loop.
Successfully tested on my machines.
Against what tree does this patch apply? It doesn't apply to Linus's git tree. Also, in the future, could you use the -p option to diff so we can see what function or data structure is being modified? It really helps in reviewing.

As mentioned earlier in this thread... when I started developing the patch I used 2.6.32.2 (latest stable from kernel.org), which might be already "old" ... sorry for that. Ok, next time I will use -p, too. For the moment, it was diff with -Nur. Shall I resend?

Best regards,
Daniel

David Miller

maintainer Applied

2010-01-14 04:40 UTC #12 lore raw

In reply to Daniel Borkmann

From: Daniel Borkmann <[email protected]>
Date: Wed, 13 Jan 2010 01:27:30 +0100

Quoted 1 line

Signed-off-by Daniel Borkmann <[email protected]>

Applied, thanks Daniel.