Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] netpoll: allow execution of multiple rx_hooks per interface

0 views
Skip to first unread message

Daniel Borkmann

unread,
Jan 6, 2010, 4:00:01 PM1/6/10
to
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/

signature.asc
netpoll_multiple_rx_hooks.patch

Matt Mackall

unread,
Jan 6, 2010, 11:00:01 PM1/6/10
to

(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.

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

David Miller

unread,
Jan 7, 2010, 4:10:02 AM1/7/10
to
From: Matt Mackall <m...@selenic.com>
Date: Wed, 06 Jan 2010 21:54:05 -0600

> 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.
>

Agreed on all counts.

Daniel Borkmann

unread,
Jan 7, 2010, 2:10:01 PM1/7/10
to
David Miller wrote:
> From: Matt Mackall <m...@selenic.com>
> Date: Wed, 06 Jan 2010 21:54:05 -0600
>
>> 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.
>>
>
> 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.

signature.asc

Daniel Borkmann

unread,
Jan 7, 2010, 7:30:02 PM1/7/10
to
Daniel Borkmann wrote:
> David Miller wrote:
>> From: Matt Mackall <m...@selenic.com>
>> Date: Wed, 06 Jan 2010 21:54:05 -0600
>>
>>> 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.
>>>
>> 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 ;)

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 <danbo...@googlemail.com>

diff -Nur a/include/linux/netpoll.h b/include/linux/netpoll.h
--- a/include/linux/netpoll.h 2010-01-05 23:52:58.000000000 +0100
+++ b/include/linux/netpoll.h 2010-01-07 23:19:25.000000000 +0100
@@ -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)
diff -Nur a/net/core/netpoll.c b/net/core/netpoll.c
--- a/net/core/netpoll.c 2010-01-05 23:53:07.000000000 +0100
+++ b/net/core/netpoll.c 2010-01-08 00:59:19.000000000 +0100
@@ -407,107 +407,119 @@
__be32 sip, tip;
unsigned char *sha;
struct sk_buff *send_skb;
- struct netpoll *np = NULL;
-
- if (npinfo->rx_np && npinfo->rx_np->dev == skb->dev)
- np = npinfo->rx_np;
- if (!np)
- return;
-
- /* No arp on this interface */
- if (skb->dev->flags & IFF_NOARP)
- return;
+ struct netpoll *np, *tmp;
+ unsigned long flags;

- if (!pskb_may_pull(skb, arp_hdr_len(skb->dev)))
+ if (list_empty(&npinfo->rx_np))
return;

- skb_reset_network_header(skb);
- skb_reset_transport_header(skb);
- arp = arp_hdr(skb);
-
- if ((arp->ar_hrd != htons(ARPHRD_ETHER) &&
- arp->ar_hrd != htons(ARPHRD_IEEE802)) ||
- arp->ar_pro != htons(ETH_P_IP) ||
- arp->ar_op != htons(ARPOP_REQUEST))
- return;
+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ list_for_each_entry_safe(np, tmp, &npinfo->rx_np, rx) {
+ if (np->dev != skb->dev)
+ continue;
+
+ /* No arp on this interface */
+ if (skb->dev->flags & IFF_NOARP)
+ continue;
+
+ if (!pskb_may_pull(skb, arp_hdr_len(skb->dev)))
+ continue;
+
+ skb_reset_network_header(skb);
+ skb_reset_transport_header(skb);
+ arp = arp_hdr(skb);
+
+ if ((arp->ar_hrd != htons(ARPHRD_ETHER) &&
+ arp->ar_hrd != htons(ARPHRD_IEEE802)) ||
+ arp->ar_pro != htons(ETH_P_IP) ||
+ arp->ar_op != htons(ARPOP_REQUEST))
+ continue;
+
+ arp_ptr = (unsigned char *)(arp+1);
+ /* save the location of the src hw addr */
+ sha = arp_ptr;
+ arp_ptr += skb->dev->addr_len;
+ memcpy(&sip, arp_ptr, 4);
+ arp_ptr += 4;
+ /*
+ * if we actually cared about dst hw addr,
+ * it would get copied here
+ */
+ arp_ptr += skb->dev->addr_len;
+ memcpy(&tip, arp_ptr, 4);

- arp_ptr = (unsigned char *)(arp+1);
- /* save the location of the src hw addr */
- sha = arp_ptr;
- arp_ptr += skb->dev->addr_len;
- memcpy(&sip, arp_ptr, 4);
- arp_ptr += 4;
- /* if we actually cared about dst hw addr, it would get copied here */
- arp_ptr += skb->dev->addr_len;
- memcpy(&tip, arp_ptr, 4);
-
- /* Should we ignore arp? */
- if (tip != np->local_ip ||
- ipv4_is_loopback(tip) || ipv4_is_multicast(tip))
- return;
+ /* Should we ignore arp? */
+ if (tip != np->local_ip ||
+ ipv4_is_loopback(tip) || ipv4_is_multicast(tip))
+ continue;
+
+ size = arp_hdr_len(skb->dev);
+ send_skb = find_skb(np, size + LL_ALLOCATED_SPACE(np->dev),
+ LL_RESERVED_SPACE(np->dev));
+
+ if (!send_skb)
+ continue;
+
+ skb_reset_network_header(send_skb);
+ arp = (struct arphdr *) skb_put(send_skb, size);
+ send_skb->dev = skb->dev;
+ send_skb->protocol = htons(ETH_P_ARP);
+
+ /* Fill the device header for the ARP frame */
+ if (dev_hard_header(send_skb, skb->dev, ptype,
+ sha, np->dev->dev_addr,
+ send_skb->len) < 0) {
+ kfree_skb(send_skb);
+ continue;
+ }

- size = arp_hdr_len(skb->dev);
- send_skb = find_skb(np, size + LL_ALLOCATED_SPACE(np->dev),
- LL_RESERVED_SPACE(np->dev));
+ /*
+ * Fill out the arp protocol part.
+ *
+ * we only support ethernet device type,
+ * which (according to RFC 1390) should
+ * always equal 1 (Ethernet).
+ */

- if (!send_skb)
- return;
+ arp->ar_hrd = htons(np->dev->type);
+ arp->ar_pro = htons(ETH_P_IP);
+ arp->ar_hln = np->dev->addr_len;
+ arp->ar_pln = 4;
+ arp->ar_op = htons(type);
+
+ arp_ptr = (unsigned char *)(arp + 1);
+ memcpy(arp_ptr, np->dev->dev_addr, np->dev->addr_len);
+ arp_ptr += np->dev->addr_len;
+ memcpy(arp_ptr, &tip, 4);
+ arp_ptr += 4;
+ memcpy(arp_ptr, sha, np->dev->addr_len);
+ arp_ptr += np->dev->addr_len;
+ memcpy(arp_ptr, &sip, 4);

- skb_reset_network_header(send_skb);
- arp = (struct arphdr *) skb_put(send_skb, size);
- send_skb->dev = skb->dev;
- send_skb->protocol = htons(ETH_P_ARP);
-
- /* Fill the device header for the ARP frame */
- if (dev_hard_header(send_skb, skb->dev, ptype,
- sha, np->dev->dev_addr,
- send_skb->len) < 0) {
- kfree_skb(send_skb);
- return;
+ netpoll_send_skb(np, send_skb);
}
-
- /*
- * Fill out the arp protocol part.
- *
- * we only support ethernet device type,
- * which (according to RFC 1390) should always equal 1 (Ethernet).
- */
-
- arp->ar_hrd = htons(np->dev->type);
- arp->ar_pro = htons(ETH_P_IP);
- arp->ar_hln = np->dev->addr_len;
- arp->ar_pln = 4;
- arp->ar_op = htons(type);
-
- arp_ptr=(unsigned char *)(arp + 1);
- memcpy(arp_ptr, np->dev->dev_addr, np->dev->addr_len);
- arp_ptr += np->dev->addr_len;
- memcpy(arp_ptr, &tip, 4);
- arp_ptr += 4;
- memcpy(arp_ptr, sha, np->dev->addr_len);
- arp_ptr += np->dev->addr_len;
- memcpy(arp_ptr, &sip, 4);
-
- netpoll_send_skb(np, send_skb);
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);
}

int __netpoll_rx(struct sk_buff *skb)
{
int proto, len, ulen;
+ int hits = 0;
struct iphdr *iph;
struct udphdr *uh;
- struct netpoll_info *npi = skb->dev->npinfo;


- struct netpoll *np = npi->rx_np;

+ struct netpoll_info *npinfo = skb->dev->npinfo;
+ struct netpoll *np, *tmp;

- if (!np)
+ if (list_empty(&npinfo->rx_np))
goto out;
+
if (skb->dev->type != ARPHRD_ETHER)
goto out;

/* check if netpoll clients need ARP */
if (skb->protocol == htons(ETH_P_ARP) &&
atomic_read(&trapped)) {
- skb_queue_tail(&npi->arp_tx, skb);
+ skb_queue_tail(&npinfo->arp_tx, skb);
return 1;
}

@@ -551,16 +563,23 @@
goto out;
if (checksum_udp(skb, uh, ulen, iph->saddr, iph->daddr))
goto out;
- if (np->local_ip && np->local_ip != iph->daddr)
- goto out;
- if (np->remote_ip && np->remote_ip != iph->saddr)
- goto out;
- if (np->local_port && np->local_port != ntohs(uh->dest))
- goto out;

- np->rx_hook(np, ntohs(uh->source),
- (char *)(uh+1),
- ulen - sizeof(struct udphdr));
+ list_for_each_entry_safe(np, tmp, &npinfo->rx_np, rx) {
+ if (np->local_ip && np->local_ip != iph->daddr)
+ continue;
+ if (np->remote_ip && np->remote_ip != iph->saddr)
+ continue;
+ if (np->local_port && np->local_port != ntohs(uh->dest))
+ continue;
+
+ np->rx_hook(np, ntohs(uh->source),
+ (char *)(uh+1),
+ ulen - sizeof(struct udphdr));
+ hits++;
+ }
+
+ if (!hits)
+ goto out;

kfree_skb(skb);
return 1;
@@ -684,6 +703,7 @@
struct net_device *ndev = NULL;
struct in_device *in_dev;
struct netpoll_info *npinfo;
+ struct netpoll *npe, *tmp;
unsigned long flags;
int err;

@@ -704,7 +724,7 @@
}

npinfo->rx_flags = 0;
- npinfo->rx_np = NULL;
+ INIT_LIST_HEAD(&npinfo->rx_np);

spin_lock_init(&npinfo->rx_lock);
skb_queue_head_init(&npinfo->arp_tx);
@@ -785,7 +805,7 @@
if (np->rx_hook) {
spin_lock_irqsave(&npinfo->rx_lock, flags);
npinfo->rx_flags |= NETPOLL_RX_ENABLED;
- npinfo->rx_np = np;
+ list_add_tail(&np->rx, &npinfo->rx_np);
spin_unlock_irqrestore(&npinfo->rx_lock, flags);
}

@@ -801,9 +821,16 @@
return 0;

release:
- if (!ndev->npinfo)
+ if (!ndev->npinfo) {
+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) {
+ npe->dev = NULL;
+ }
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+
kfree(npinfo);
- np->dev = NULL;
+ }
+
dev_put(ndev);
return err;
}
@@ -823,10 +850,11 @@
if (np->dev) {
npinfo = np->dev->npinfo;
if (npinfo) {
- if (npinfo->rx_np == np) {
+ if (!list_empty(&npinfo->rx_np)) {
spin_lock_irqsave(&npinfo->rx_lock, flags);
- npinfo->rx_np = NULL;
- npinfo->rx_flags &= ~NETPOLL_RX_ENABLED;
+ list_del(&np->rx);
+ if (list_empty(&npinfo->rx_np))
+ npinfo->rx_flags &= ~NETPOLL_RX_ENABLED;
spin_unlock_irqrestore(&npinfo->rx_lock, flags);
}

signature.asc

Matt Mackall

unread,
Jan 11, 2010, 6:30:02 PM1/11/10
to
On Fri, 2010-01-08 at 01:20 +0100, Daniel Borkmann wrote:
> Daniel Borkmann wrote:
> > David Miller wrote:
> >> From: Matt Mackall <m...@selenic.com>
> >> Date: Wed, 06 Jan 2010 21:54:05 -0600
> >>
> >>> 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.
> >>>
> >> 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 ;)
>
> 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 <m...@selenic.com>

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

David Miller

unread,
Jan 11, 2010, 7:00:02 PM1/11/10
to
From: Matt Mackall <m...@selenic.com>
Date: Mon, 11 Jan 2010 17:21:48 -0600

> Looks pretty good. Dave?
>
> Acked-by: Matt Mackall <m...@selenic.com>

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.

Daniel Borkmann

unread,
Jan 11, 2010, 7:10:02 PM1/11/10
to
Matt Mackall wrote:

> On Mon, 2010-01-11 at 15:59 -0800, David Miller wrote:
>> From: Matt Mackall <m...@selenic.com>
>> Date: Mon, 11 Jan 2010 17:21:48 -0600
>>
>>> Looks pretty good. Dave?
>>>
>>> Acked-by: Matt Mackall <m...@selenic.com>
>> 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

signature.asc

Matt Mackall

unread,
Jan 11, 2010, 7:10:02 PM1/11/10
to
On Mon, 2010-01-11 at 15:59 -0800, David Miller wrote:
> From: Matt Mackall <m...@selenic.com>
> Date: Mon, 11 Jan 2010 17:21:48 -0600
>
> > Looks pretty good. Dave?
> >
> > Acked-by: Matt Mackall <m...@selenic.com>
>
> 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.

--

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

Daniel Borkmann

unread,
Jan 12, 2010, 7:30:02 PM1/12/10
to

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 <danbo...@googlemail.com>

+++ b/net/core/netpoll.c 2010-01-12 23:36:48.000000000 +0100
@@ -407,11 +407,24 @@


__be32 sip, tip;
unsigned char *sha;
struct sk_buff *send_skb;

- struct netpoll *np = NULL;


+ struct netpoll *np, *tmp;
+ unsigned long flags;

+ int hits = 0;

+
+ if (list_empty(&npinfo->rx_np))
+ return;
+
+ /* Before checking the packet, we do some early
+ inspection whether this is interesting at all */


+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ list_for_each_entry_safe(np, tmp, &npinfo->rx_np, rx) {

+ if (np->dev == skb->dev)
+ hits++;


+ }
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);

- if (npinfo->rx_np && npinfo->rx_np->dev == skb->dev)
- np = npinfo->rx_np;
- if (!np)
+ /* No netpoll struct is using this dev */
+ if (!hits)
return;

/* No arp on this interface */

@@ -437,77 +450,91 @@


arp_ptr += skb->dev->addr_len;

memcpy(&sip, arp_ptr, 4);


arp_ptr += 4;
- /* if we actually cared about dst hw addr, it would get copied here */

+ /* If we actually cared about dst hw addr,
+ it would get copied here */


arp_ptr += skb->dev->addr_len;

memcpy(&tip, arp_ptr, 4);

/* Should we ignore arp? */
- if (tip != np->local_ip ||
- ipv4_is_loopback(tip) || ipv4_is_multicast(tip))

+ if (ipv4_is_loopback(tip) || ipv4_is_multicast(tip))
return;

size = arp_hdr_len(skb->dev);
- send_skb = find_skb(np, size + LL_ALLOCATED_SPACE(np->dev),
- LL_RESERVED_SPACE(np->dev));

-


- if (!send_skb)
- return;

-


- skb_reset_network_header(send_skb);
- arp = (struct arphdr *) skb_put(send_skb, size);
- send_skb->dev = skb->dev;
- send_skb->protocol = htons(ETH_P_ARP);
-
- /* Fill the device header for the ARP frame */
- if (dev_hard_header(send_skb, skb->dev, ptype,
- sha, np->dev->dev_addr,
- send_skb->len) < 0) {
- kfree_skb(send_skb);
- return;

- }

- /*
- * Fill out the arp protocol part.
- *
- * we only support ethernet device type,
- * which (according to RFC 1390) should always equal 1 (Ethernet).
- */

+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ list_for_each_entry_safe(np, tmp, &npinfo->rx_np, rx) {

+ if (tip != np->local_ip)
+ continue;
+


+ send_skb = find_skb(np, size + LL_ALLOCATED_SPACE(np->dev),
+ LL_RESERVED_SPACE(np->dev));
+ if (!send_skb)
+ continue;
+
+ skb_reset_network_header(send_skb);
+ arp = (struct arphdr *) skb_put(send_skb, size);
+ send_skb->dev = skb->dev;
+ send_skb->protocol = htons(ETH_P_ARP);
+
+ /* Fill the device header for the ARP frame */
+ if (dev_hard_header(send_skb, skb->dev, ptype,
+ sha, np->dev->dev_addr,
+ send_skb->len) < 0) {
+ kfree_skb(send_skb);
+ continue;
+ }

- arp->ar_hrd = htons(np->dev->type);


- arp->ar_pro = htons(ETH_P_IP);
- arp->ar_hln = np->dev->addr_len;
- arp->ar_pln = 4;
- arp->ar_op = htons(type);
-
- arp_ptr=(unsigned char *)(arp + 1);
- memcpy(arp_ptr, np->dev->dev_addr, np->dev->addr_len);
- arp_ptr += np->dev->addr_len;
- memcpy(arp_ptr, &tip, 4);
- arp_ptr += 4;
- memcpy(arp_ptr, sha, np->dev->addr_len);
- arp_ptr += np->dev->addr_len;
- memcpy(arp_ptr, &sip, 4);

+ /*
+ * Fill out the arp protocol part.
+ *
+ * we only support ethernet device type,
+ * which (according to RFC 1390) should
+ * always equal 1 (Ethernet).
+ */

- netpoll_send_skb(np, send_skb);


+ arp->ar_hrd = htons(np->dev->type);
+ arp->ar_pro = htons(ETH_P_IP);
+ arp->ar_hln = np->dev->addr_len;
+ arp->ar_pln = 4;
+ arp->ar_op = htons(type);
+
+ arp_ptr = (unsigned char *)(arp + 1);
+ memcpy(arp_ptr, np->dev->dev_addr, np->dev->addr_len);
+ arp_ptr += np->dev->addr_len;
+ memcpy(arp_ptr, &tip, 4);
+ arp_ptr += 4;
+ memcpy(arp_ptr, sha, np->dev->addr_len);
+ arp_ptr += np->dev->addr_len;
+ memcpy(arp_ptr, &sip, 4);

+
+ netpoll_send_skb(np, send_skb);
+
+ /* If there are several rx_hooks for the same address,
+ we're fine by sending a single reply */
+ break;


+ }
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);
}

int __netpoll_rx(struct sk_buff *skb)


{
int proto, len, ulen;
+ int hits = 0;
struct iphdr *iph;
struct udphdr *uh;
- struct netpoll_info *npi = skb->dev->npinfo;

- struct netpoll *np = npi->rx_np;

+ struct netpoll_info *npinfo = skb->dev->npinfo;
+ struct netpoll *np, *tmp;

- if (!np)


+ if (list_empty(&npinfo->rx_np))
goto out;
+
if (skb->dev->type != ARPHRD_ETHER)
goto out;

/* check if netpoll clients need ARP */
if (skb->protocol == htons(ETH_P_ARP) &&
atomic_read(&trapped)) {
- skb_queue_tail(&npi->arp_tx, skb);
+ skb_queue_tail(&npinfo->arp_tx, skb);
return 1;
}

@@ -551,16 +578,23 @@

kfree_skb(skb);
return 1;
@@ -684,6 +718,7 @@


struct net_device *ndev = NULL;
struct in_device *in_dev;
struct netpoll_info *npinfo;
+ struct netpoll *npe, *tmp;
unsigned long flags;
int err;

@@ -704,7 +739,7 @@
}

npinfo->rx_flags = 0;
- npinfo->rx_np = NULL;
+ INIT_LIST_HEAD(&npinfo->rx_np);

spin_lock_init(&npinfo->rx_lock);
skb_queue_head_init(&npinfo->arp_tx);
@@ -785,7 +820,7 @@


if (np->rx_hook) {
spin_lock_irqsave(&npinfo->rx_lock, flags);
npinfo->rx_flags |= NETPOLL_RX_ENABLED;
- npinfo->rx_np = np;
+ list_add_tail(&np->rx, &npinfo->rx_np);
spin_unlock_irqrestore(&npinfo->rx_lock, flags);
}

@@ -801,9 +836,16 @@
return 0;

release:
- if (!ndev->npinfo)
+ if (!ndev->npinfo) {
+ spin_lock_irqsave(&npinfo->rx_lock, flags);
+ list_for_each_entry_safe(npe, tmp, &npinfo->rx_np, rx) {
+ npe->dev = NULL;
+ }
+ spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+
kfree(npinfo);
- np->dev = NULL;
+ }
+
dev_put(ndev);
return err;
}

@@ -823,10 +865,11 @@

signature.asc

Jeff Moyer

unread,
Jan 13, 2010, 9:00:01 AM1/13/10
to
Daniel Borkmann <danbo...@googlemail.com> writes:

> Daniel Borkmann wrote:
>> Matt Mackall wrote:
>>> On Mon, 2010-01-11 at 15:59 -0800, David Miller wrote:
>>>> From: Matt Mackall <m...@selenic.com>
>>>> Date: Mon, 11 Jan 2010 17:21:48 -0600
>>>>
>>>>> Looks pretty good. Dave?
>>>>>
>>>>> Acked-by: Matt Mackall <m...@selenic.com>
>>>> 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.
>
> 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

unread,
Jan 13, 2010, 12:10:02 PM1/13/10
to
2010/1/13 Jeff Moyer <jmo...@redhat.com>:

> Daniel Borkmann <danbo...@googlemail.com> writes:
>
>> Daniel Borkmann wrote:
>>> Matt Mackall wrote:
>>>> On Mon, 2010-01-11 at 15:59 -0800, David Miller wrote:
>>>>> From: Matt Mackall <m...@selenic.com>
>>>>> Date: Mon, 11 Jan 2010 17:21:48 -0600
>>>>>
>>>>>> Looks pretty good. Dave?
>>>>>>
>>>>>> Acked-by: Matt Mackall <m...@selenic.com>
>>>>> 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.
>>
>> 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

unread,
Jan 13, 2010, 11:50:02 PM1/13/10
to
From: Daniel Borkmann <danbo...@googlemail.com>
Date: Wed, 13 Jan 2010 01:27:30 +0100

> Signed-off-by: Daniel Borkmann <danbo...@googlemail.com>

Applied, thanks Daniel.

0 new messages