Netdev List
 help / color / mirror / Atom feed
* [patch 1/3] netpoll: set poll_owner to -1 before unlocking in netpoll_poll_unlock
From: Jeff Moyer @ 2005-06-23  1:27 UTC (permalink / raw)
  To: mpm, netdev, linux-kernel, akpm
In-Reply-To: <17082.4037.875432.648439@segfault.boston.redhat.com>

Hi,

This trivial patch moves the assignment of poll_owner to -1 inside of the
lock.  This fixes a potential SMP race in the code.

-Jeff

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---

--- linux-2.6.12/include/linux/netpoll.h.orig	2005-06-22 18:47:12.917261688 -0400
+++ linux-2.6.12/include/linux/netpoll.h	2005-06-22 18:47:15.799783018 -0400
@@ -53,8 +53,8 @@ static inline void netpoll_poll_lock(str
 static inline void netpoll_poll_unlock(struct net_device *dev)
 {
 	if (dev->np) {
-		spin_unlock(&dev->np->poll_lock);
 		dev->np->poll_owner = -1;
+		spin_unlock(&dev->np->poll_lock);
 	}
 }
 

^ permalink raw reply

* [patch 0/3] netpoll: support multiple netpoll clients per net_device
From: Jeff Moyer @ 2005-06-23  1:26 UTC (permalink / raw)
  To: mpm; +Cc: netdev, linux-kernel, akpm

Hi,

This patch series restores the ability to register multiple netpoll clients
against the same network interface.  To this end, I created a new structure:

struct netpoll_info {
	spinlock_t poll_lock;
	int poll_owner;
	int rx_flags;
	spinlock_t rx_lock;
	struct netpoll *rx_np; /* netpoll that registered an rx_hook */
};

This is the structure which gets pointed to by the net_device.  All of the
flags and locks which are specific to the INTERFACE go here.  Any variables
which must be kept per struct netpoll were left in the struct netpoll.  So
now, we have a cleaner separation of data and its scope.

Since we never really supported having more than one struct netpoll
register an rx_hook, I did not re-implement the rx_list.  Instead, there is
a single pointer in the netpoll_info structure (np_rx).  Note that if we
decide that we would like to register multiple rx_hooks per net_device,
this architecture does not preclude such extension.

We need to protect setting and clearing of the rx_np pointer, so I added a
lock (rx_lock) to the netpoll_info struct. [1]  There is one lock per
struct net_device, and I am certain that it will be low contention, as rx_np
will only be changed during an insmod or rmmod.

In the process of making these changes, I fixed a bug in netpoll_poll_unlock.
The function released the lock before setting the poll_owner to -1.


I have tested this by registering two netpoll clients, and verifying that
they both function properly.  The clients were netconsole, and a quick
module I hacked together to send console messages to syslog.  I issued
sysrq-h, sysrq-m, and sysrq-t's both by echo'ing to /proc/sysrq-trigger and
by hitting the key combination on the keyboard.  This verifies that the
modules work both inside and out of interrupt context.

Cheers,

Jeff

[1] Matt notes that we may be able to merge this lock with the poll_lock in
    the future.

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-22 23:23 UTC (permalink / raw)
  To: David S. Miller
  Cc: ak, leonid.grossman, davem, hadi, becker, rick.jones2, netdev
In-Reply-To: <20050622.191956.39166724.davem@redhat.com>

On Wed, Jun 22, 2005 at 07:19:56PM -0400, David S. Miller wrote:
> From: Andi Kleen <ak@suse.de>
> Date: Thu, 23 Jun 2005 01:13:00 +0200
> 
> > The computing time must be quite long to be really a win.
> > You need to waste a few hundred cycles at least on a modern fast CPU.
> 
> SKB allocation more than fits this requirement, and that is exactly
> what the RX descriptor replenishment will do.

It shouldn't in theory. Unless they did something bad to the slab
allocator again when I wasn't looking ;-) 

> 
> Even if SKB allocation was only half the necessary number of cycles
> for the prefetch to hit the cpu, it'd still be a win.

If it's too small then it might be left in the noise.

-Andi

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: David S. Miller @ 2005-06-22 23:19 UTC (permalink / raw)
  To: ak; +Cc: leonid.grossman, davem, hadi, becker, rick.jones2, netdev
In-Reply-To: <20050622231300.GC14251@wotan.suse.de>

From: Andi Kleen <ak@suse.de>
Date: Thu, 23 Jun 2005 01:13:00 +0200

> The computing time must be quite long to be really a win.
> You need to waste a few hundred cycles at least on a modern fast CPU.

SKB allocation more than fits this requirement, and that is exactly
what the RX descriptor replenishment will do.

Even if SKB allocation was only half the necessary number of cycles
for the prefetch to hit the cpu, it'd still be a win.

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-22 23:13 UTC (permalink / raw)
  To: Leonid Grossman
  Cc: 'David S. Miller', ak, hadi, becker, rick.jones2, netdev,
	davem
In-Reply-To: <200506222242.j5MMgbxS009935@guinness.s2io.com>

> This is very hw-dependent, since there are NICs that read descriptors in
> batches anyways - but the second argument below is compelling.

The computing time must be quite long to be really a win.
You need to waste a few hundred cycles at least on a modern fast CPU.

-Andi
> > 
> > 2) As shown above, it gives you compute time which can be used to
> >    schedule the prefetch.  This nearly makes RX replenishment free.
> >    Instead of having the CPU spin on a cache miss when we run
> >    eth_type_trans() during those cycles, we do useful work.

^ permalink raw reply

* RE: RFC: NAPI packet weighting patch
From: Leonid Grossman @ 2005-06-22 22:42 UTC (permalink / raw)
  To: 'David S. Miller', ak; +Cc: hadi, becker, rick.jones2, netdev, davem
In-Reply-To: <20050622.132241.21929037.davem@davemloft.net>

 

> -----Original Message-----
> From: David S. Miller [mailto:davem@davemloft.net] 
> Sent: Wednesday, June 22, 2005 1:23 PM
> To: ak@suse.de
> Cc: leonid.grossman@neterion.com; hadi@cyberus.ca; 
> becker@scyld.com; rick.jones2@hp.com; netdev@oss.sgi.com; 
> davem@redhat.com
> Subject: Re: RFC: NAPI packet weighting patch
> 
> From: Andi Kleen <ak@suse.de>
> Date: Wed, 22 Jun 2005 20:06:55 +0200
> 
> > However it is tricky because CPUs have only a limited load queue 
> > entries and doing too many prefetches will just overflow that.
> 
> Several processors can queue about 8 prefetch requests, and 
> these slots are independant of those consumed by a load.
> 
> Yes, if you queue too many prefetches, the queue overflows.
> 
> I think the optimal scheme would be:
> 
> 1) eth_type_trans() info in RX descriptor
> 2) prefetch(skb->data) done as early as possible in driver
>    RX handling
> 
> Actually, I believe to most optimal scheme is:
> 
> foo_driver_rx()
> {
> 	for_each_rx_descriptor() {
> 		...
> 		skb = driver_priv->rx_skbs[index];
> 		prefetch(skb->data);
> 
> 		skb = realloc_or_recycle_rx_descriptor(skb, index);
> 		if (skb == NULL)
> 			goto next_rxd;
> 
> 		skb->prot = eth_type_trans(skb, driver_priv->dev);
> 		netif_receive_skb(skb);
> 		...
> 	next_rxd:
> 		...
> 	}
> }
> 
> The idea is that first the prefetch goes into flight, then 
> you do the recycle or reallocation of the RX descriptor SKB, 
> then you try to touch the data.
> 
> This makes it very likely the prefetch will be in the cpu in time.
> 
> Everyone seems to have this absolute fetish about batching 
> the RX descriptor refilling work.  It's wrong, it should be 
> done when you pull a receive packet off the ring, for many 
> reasons.  Off the top of my head:

This is very hw-dependent, since there are NICs that read descriptors in
batches anyways - but the second argument below is compelling.

> 
> 1) Descriptors are refilled as soon as possible, decreasing
>    the chance of the device hitting the end of the RX ring
>    and thus unable to receive a packet.
> 
> 2) As shown above, it gives you compute time which can be used to
>    schedule the prefetch.  This nearly makes RX replenishment free.
>    Instead of having the CPU spin on a cache miss when we run
>    eth_type_trans() during those cycles, we do useful work.
> 
> I'm going to play around with these ideas in the tg3 driver.
> Obvious patch below.

We will play around with the s2io driver as well, there seem to be several
interesting ideas to try - thanks a lot for the input!
Cheers, Leonid

> 
> --- 1/drivers/net/tg3.c.~1~	2005-06-22 12:33:07.000000000 -0700
> +++ 2/drivers/net/tg3.c	2005-06-22 13:19:13.000000000 -0700
> @@ -2772,6 +2772,13 @@
>  			goto next_pkt_nopost;
>  		}
>  
> +		/* Prefetch now.  The recycle/realloc of the RX
> +		 * entry is moderately expensive, so by the time
> +		 * that is complete the data should have reached
> +		 * the cpu.
> +		 */
> +		prefetch(skb->data);
> +
>  		work_mask |= opaque_key;
>  
>  		if ((desc->err_vlan & RXD_ERR_MASK) != 0 &&
> 

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: David S. Miller @ 2005-06-22 22:30 UTC (permalink / raw)
  To: dada1; +Cc: ak, leonid.grossman, hadi, becker, rick.jones2, netdev, davem
In-Reply-To: <42B9E281.1090109@cosmosbay.com>

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Thu, 23 Jun 2005 00:13:21 +0200

> I also found that the memcpy() done to copy the data to the new skb suffers from misalignment.
> 
> This is because of skb_reserve(skbs, 2) that was done on both skb, and memcpy() (at least on x86_64) doing long word copies without checking 
> alignment of source or destination.
> 
> Maybe we could :
>
> 1) make sure both skbs had the same skb_reserve() of 2 (thats not clear because tg3.c mixes the '2' and tp->rx_offset,
> and according to a comment :
> 	rx_offset != 2 iff this is a 5701 card running 
>                                           	in PCI-X mode
>
> 2) and do :
> 
> -	memcpy(copy_skb->data, skb->data, len);
> +	memcpy(copy_skb->data-2, skb->data-2, len+2);
> 
> (That is copy 2 more bytes, but gain aligned copy to speedup memcpy())

Yep, good idea.  Actually, the driver should be using
NET_IP_ALIGN for rx_offset unless it's the 5701 card running
in PCI-X mode case.

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: David S. Miller @ 2005-06-22 22:23 UTC (permalink / raw)
  To: dada1; +Cc: ak, leonid.grossman, hadi, becker, rick.jones2, netdev, davem
In-Reply-To: <42B9DA4D.5090103@cosmosbay.com>

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Wed, 22 Jun 2005 23:38:21 +0200

> Then maybe we could also play with prefetchw() in the case the
> incoming frame is small enough to be copied to a new skb.

That's a good idea too.  In fact, this would deal with platforms
that use non-temporal stores in their memcpy() implementation.

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Eric Dumazet @ 2005-06-22 22:13 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David S. Miller, ak, leonid.grossman, hadi, becker, rick.jones2,
	netdev, davem
In-Reply-To: <42B9DA4D.5090103@cosmosbay.com>

Eric Dumazet a écrit :

> 
> Then maybe we could also play with prefetchw() in the case the incoming 
> frame
> is small enough to be copied to a new skb.
> 
> drivers/net/tg3.c
> 
>     copy_skb = dev_alloc_skb(len + 2);
>     if (copy_skb == NULL)
>         goto drop_it_no_recycle;
> +    prefetchw(copy_skb->data);
> 
>     copy_skb->dev = tp->dev;
>     skb_reserve(copy_skb, 2);
>     skb_put(copy_skb, len);
> 
> 
> 

I also found that the memcpy() done to copy the data to the new skb suffers from misalignment.

This is because of skb_reserve(skbs, 2) that was done on both skb, and memcpy() (at least on x86_64) doing long word copies without checking 
alignment of source or destination.

Maybe we could :

1) make sure both skbs had the same skb_reserve() of 2 (thats not clear because tg3.c mixes the '2' and tp->rx_offset,
and according to a comment :
	rx_offset != 2 iff this is a 5701 card running 
                                          	in PCI-X mode

2) and do :

-	memcpy(copy_skb->data, skb->data, len);
+	memcpy(copy_skb->data-2, skb->data-2, len+2);

(That is copy 2 more bytes, but gain aligned copy to speedup memcpy())

Eric Dumazet

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-22 22:11 UTC (permalink / raw)
  To: Chris Friesen
  Cc: Andi Kleen, David S. Miller, leonid.grossman, hadi, becker,
	rick.jones2, netdev
In-Reply-To: <42B9DDDA.5040405@nortel.com>

On Wed, Jun 22, 2005 at 03:53:30PM -0600, Chris Friesen wrote:
> Andi Kleen wrote:
> 
> >8 entries? That sounds very small. Is that an old Sparc or something? :)
> 
> The G5 has 8 prefetch streams.  Not an ancient cpu.

prefetch stream means a context of the auto prefetcher. 

It different from a load queue entry which is just a load of a cache line
which can be triggered by user instructions or the auto prefetcher. 
Each prefetch stream would consume a lot of them, so just for your 8 streams 
above you probably need a large two digit number or more.

I don't have exact numbers for the PPC970, but afaik its LS unit
has a very long queue. On POWER4 (which is a very similar CPU) we see 
a lot of races that don't happen on other platforms. That seems to be 
because it reorders writes every aggressively. I suppose this is true for
reads as well.

-Andi

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Chris Friesen @ 2005-06-22 21:53 UTC (permalink / raw)
  To: Andi Kleen
  Cc: David S. Miller, leonid.grossman, hadi, becker, rick.jones2,
	netdev
In-Reply-To: <20050622211058.GY14251@wotan.suse.de>

Andi Kleen wrote:

> 8 entries? That sounds very small. Is that an old Sparc or something? :)

The G5 has 8 prefetch streams.  Not an ancient cpu.


Chris

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Eric Dumazet @ 2005-06-22 21:38 UTC (permalink / raw)
  To: David S. Miller
  Cc: ak, leonid.grossman, hadi, becker, rick.jones2, netdev, davem
In-Reply-To: <20050622.132241.21929037.davem@davemloft.net>

David S. Miller a écrit :

> 
> 2) As shown above, it gives you compute time which can be used to
>    schedule the prefetch.  This nearly makes RX replenishment free.
>    Instead of having the CPU spin on a cache miss when we run
>    eth_type_trans() during those cycles, we do useful work.
> 
> I'm going to play around with these ideas in the tg3 driver.
> Obvious patch below.


Then maybe we could also play with prefetchw() in the case the incoming frame
is small enough to be copied to a new skb.

drivers/net/tg3.c

	copy_skb = dev_alloc_skb(len + 2);
	if (copy_skb == NULL)
		goto drop_it_no_recycle;
+	prefetchw(copy_skb->data);

	copy_skb->dev = tp->dev;
	skb_reserve(copy_skb, 2);
	skb_put(copy_skb, len);

^ permalink raw reply

* Re: [patch,rfc] allow registration of multiple netpolls per interface
From: Matt Mackall @ 2005-06-22 21:27 UTC (permalink / raw)
  To: Jeff Moyer; +Cc: netdev, netdev, linux-kernel
In-Reply-To: <17081.53899.201190.106025@segfault.boston.redhat.com>

On Wed, Jun 22, 2005 at 05:05:15PM -0400, Jeff Moyer wrote:
> mpm> It might be simpler to have a single lock here..?
> >> 
> >> Maybe.  You can't really have netpoll code running on multiple cpus at the
> >> same time, right?  This is the rx path, remember, so the other cpu should
> >> be spinning on the poll_lock.
> >> 
> >> Keeping separate locks would allow you to unregister a struct netpoll
> >> associated with another net device without causing lock contention.  This
> >> is a very minor win, obviously.
> >> 
> >> I still feel like this npinfo struct is the right place for this, though.
> >> If you're strongly opposed to that, I'll change it.
> 
> mpm> No, certainly having it in npinfo makes sense. I just was wondering if
> mpm> we really need two locks in there.
> 
> Oh, I misunderstood.  Well, one protects recursing into the driver's poll
> routine, the other protects access to the np_rx pointer, which may later
> become a list.  I don't think we can lump these two together, do you?

I don't see why we couldn't, but let's worry about it later.
 
> >> >> +	spin_lock_irqsave(&npinfo->rx_lock, flags);
> >> >> +	if (npinfo->rx_np->dev == skb->dev)
> >> >> +		np = npinfo->rx_np;
> >> >> +	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
> >> 
> mpm> And I think that means we don't need the lock here either.  
> >> 
> >> Sure we do.  We need to protect against rmmod's.
> 
> mpm> How can we have an rmmmod when we're trapped?
> 
> Looking over the code, I don't see what would prevent this.  Could you
> point me the code which prevents this?

I forgot we overloaded trapped for dealing with NAPI. Formerly
trapping meant "I'm stopping the box, drop every packet that's not
addressed to me" which also implied no one should be pulling the rug
out from under us.

> (Interdiff first)

Looks fine.

-- 
Mathematics is the supreme nostalgia of our time.

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: David S. Miller @ 2005-06-22 21:16 UTC (permalink / raw)
  To: ak; +Cc: leonid.grossman, hadi, becker, rick.jones2, netdev, davem
In-Reply-To: <20050622211058.GY14251@wotan.suse.de>

From: Andi Kleen <ak@suse.de>
Date: Wed, 22 Jun 2005 23:10:58 +0200

> 8 entries? That sounds very small. Is that an old Sparc or something? :)

Hey, Sparc does suck, this isn't news for anyone :-)

> Looks reasonable. Not sure about the "most optimal" though, some benchmarking
> of different schemes would be probably a good idea.

Absolutely.

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-22 21:10 UTC (permalink / raw)
  To: David S. Miller
  Cc: ak, leonid.grossman, hadi, becker, rick.jones2, netdev, davem
In-Reply-To: <20050622.132241.21929037.davem@davemloft.net>

On Wed, Jun 22, 2005 at 01:22:41PM -0700, David S. Miller wrote:
> From: Andi Kleen <ak@suse.de>
> Date: Wed, 22 Jun 2005 20:06:55 +0200
> 
> > However it is tricky because CPUs have only a limited load queue
> > entries and doing too many prefetches will just overflow that.
> 
> Several processors can queue about 8 prefetch requests, and
> these slots are independant of those consumed by a load.

8 entries? That sounds very small. Is that an old Sparc or something? :)

An Opteron has 44 entries, effectively 32 for L2.   Netburst
or POWER4 derived CPUs have more than that.

> Yes, if you queue too many prefetches, the queue overflows.
> 
> I think the optimal scheme would be:
> 
> 1) eth_type_trans() info in RX descriptor
> 2) prefetch(skb->data) done as early as possible in driver
>    RX handling
> 
> Actually, I believe to most optimal scheme is:

Looks reasonable. Not sure about the "most optimal" though, some benchmarking
of different schemes would be probably a good idea.

-Andi

^ permalink raw reply

* Re: [patch,rfc] allow registration of multiple netpolls per interface
From: Jeff Moyer @ 2005-06-22 21:05 UTC (permalink / raw)
  To: Matt Mackall; +Cc: netdev, netdev, linux-kernel
In-Reply-To: <20050622170128.GV27572@waste.org>

==> Regarding Re: [patch,rfc] allow registration of multiple netpolls per interface; Matt Mackall <mpm@selenic.com> adds:

mpm> On Wed, Jun 22, 2005 at 07:47:37AM -0400, Jeff Moyer wrote:
mpm> Hmm. It's conceivable we'll want netdump and kgdb on the same
mpm> interface so we'll have to address this eventually..
>> 
>> Well, do you want to address it eventually, or now?  As I said, it's never
>> bitten anyone before.

mpm> Later's fine. I just don't want to design it out by accident again.
 
OK.

>> >> struct netpoll_info {
>> >>         spinlock_t poll_lock;
>> >>         int poll_owner;
>> >>         int rx_flags;
>> >>         spinlock_t rx_lock;
>> >>         struct netpoll *rx_np; /* netpoll that registered an rx_hook */
>> >> };

[snip]

mpm> It might be simpler to have a single lock here..?
>> 
>> Maybe.  You can't really have netpoll code running on multiple cpus at the
>> same time, right?  This is the rx path, remember, so the other cpu should
>> be spinning on the poll_lock.
>> 
>> Keeping separate locks would allow you to unregister a struct netpoll
>> associated with another net device without causing lock contention.  This
>> is a very minor win, obviously.
>> 
>> I still feel like this npinfo struct is the right place for this, though.
>> If you're strongly opposed to that, I'll change it.

mpm> No, certainly having it in npinfo makes sense. I just was wondering if
mpm> we really need two locks in there.

Oh, I misunderstood.  Well, one protects recursing into the driver's poll
routine, the other protects access to the np_rx pointer, which may later
become a list.  I don't think we can lump these two together, do you?

>> >> +	spin_lock_irqsave(&npinfo->rx_lock, flags);
>> >> +	if (npinfo->rx_np->dev == skb->dev)
>> >> +		np = npinfo->rx_np;
>> >> +	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
>> 
mpm> And I think that means we don't need the lock here either.  
>> 
>> Sure we do.  We need to protect against rmmod's.

mpm> How can we have an rmmmod when we're trapped?

Looking over the code, I don't see what would prevent this.  Could you
point me the code which prevents this?

It's a good thing we're discussing it, since I found that I didn't take the
lock in netpoll_cleanup.


Okay, so here's the full patch again, with the changes we've discussed.
I've also included an interdiff.  As you can see, the first version I sent
didn't have some basic compile fixes, sorry about that.  Anyway, I have
booted and tested this version with multiple netpoll clients.

Barring any negative feedback, I'll break this up and send it as a patch
series.

Thanks,

Jeff

(Interdiff first)

diff -u linux-2.6.12-rc6/net/core/netpoll.c linux-2.6.12-rc6/net/core/netpoll.c
--- linux-2.6.12-rc6/net/core/netpoll.c	2005-06-21 16:03:22.409620400 -0400
+++ linux-2.6.12-rc6/net/core/netpoll.c	2005-06-22 16:51:24.336062231 -0400
@@ -130,8 +130,8 @@
  */
 static void poll_napi(struct netpoll *np)
 {
-	int budget = 16;
 	struct netpoll_info *npinfo = np->dev->npinfo;
+	int budget = 16;
 
 	if (test_bit(__LINK_STATE_RX_SCHED, &np->dev->state) &&
 	    npinfo->poll_owner != smp_processor_id() &&
@@ -344,22 +344,22 @@
 
 static void arp_reply(struct sk_buff *skb)
 {
+	struct netpoll_info *npinfo = skb->dev->npinfo;
 	struct arphdr *arp;
 	unsigned char *arp_ptr;
 	int size, type = ARPOP_REPLY, ptype = ETH_P_ARP;
 	u32 sip, tip;
+	unsigned long flags;
 	struct sk_buff *send_skb;
-	struct netpoll *np;
-	struct netpoll_info *npinfo = skb->dev->npinfo;
-
-	if (!npinfo) return;
+	struct netpoll *np = NULL;
 
 	spin_lock_irqsave(&npinfo->rx_lock, flags);
-	if (npinfo->rx_np->dev == skb->dev)
+	if (npinfo->rx_np && npinfo->rx_np->dev == skb->dev)
 		np = npinfo->rx_np;
 	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 
-	if (!np) return;
+	if (!np)
+		return;
 
 	/* No arp on this interface */
 	if (skb->dev->flags & IFF_NOARP)
@@ -716,7 +716,7 @@
 		spin_lock_irqsave(&npinfo->rx_lock, flags);
 		npinfo->rx_flags |= NETPOLL_RX_ENABLED;
 		npinfo->rx_np = np;
-		spin_unlock_irqsave(&npinfo->rx_lock, flags);
+		spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 	}
 	/* last thing to do is link it to the net device structure */
 	ndev->npinfo = npinfo;
@@ -734,12 +734,15 @@
 void netpoll_cleanup(struct netpoll *np)
 {
 	struct netpoll_info *npinfo;
+	unsigned long flags;
 
 	if (np->dev) {
 		npinfo = np->dev->npinfo;
 		if (npinfo && npinfo->rx_np == np) {
+			spin_lock_irqsave(&npinfo->rx_lock, flags);
 			npinfo->rx_np = NULL;
 			npinfo->rx_flags &= ~NETPOLL_RX_ENABLED;
+			spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 		}
 		dev_put(np->dev);
 	}
reverted:
--- linux-2.6.12-rc6/net/core/dev.c	2005-06-21 13:53:51.583407710 -0400
+++ linux-2.6.12-rc6/net/core/dev.c.orig	2005-06-20 19:51:59.000000000 -0400
@@ -1656,7 +1656,6 @@
 	unsigned short type;
 
 	/* if we've gotten here through NAPI, check netpoll */
-	/* how else can we get here?  --phro */
 	if (skb->dev->poll && netpoll_rx(skb))
 		return NET_RX_DROP;
 


And now, the full diff:

--- linux-2.6.12-rc6/net/core/netpoll.c.orig	2005-06-20 19:51:56.000000000 -0400
+++ linux-2.6.12-rc6/net/core/netpoll.c	2005-06-22 16:51:24.336062231 -0400
@@ -130,19 +130,20 @@ static int checksum_udp(struct sk_buff *
  */
 static void poll_napi(struct netpoll *np)
 {
+	struct netpoll_info *npinfo = np->dev->npinfo;
 	int budget = 16;
 
 	if (test_bit(__LINK_STATE_RX_SCHED, &np->dev->state) &&
-	    np->poll_owner != smp_processor_id() &&
-	    spin_trylock(&np->poll_lock)) {
-		np->rx_flags |= NETPOLL_RX_DROP;
+	    npinfo->poll_owner != smp_processor_id() &&
+	    spin_trylock(&npinfo->poll_lock)) {
+		npinfo->rx_flags |= NETPOLL_RX_DROP;
 		atomic_inc(&trapped);
 
 		np->dev->poll(np->dev, &budget);
 
 		atomic_dec(&trapped);
-		np->rx_flags &= ~NETPOLL_RX_DROP;
-		spin_unlock(&np->poll_lock);
+		npinfo->rx_flags &= ~NETPOLL_RX_DROP;
+		spin_unlock(&npinfo->poll_lock);
 	}
 }
 
@@ -245,6 +246,7 @@ repeat:
 static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 {
 	int status;
+	struct netpoll_info *npinfo;
 
 repeat:
 	if(!np || !np->dev || !netif_running(np->dev)) {
@@ -253,7 +255,8 @@ repeat:
 	}
 
 	/* avoid recursion */
-	if(np->poll_owner == smp_processor_id() ||
+	npinfo = np->dev->npinfo;
+	if(npinfo->poll_owner == smp_processor_id() ||
 	   np->dev->xmit_lock_owner == smp_processor_id()) {
 		if (np->drop)
 			np->drop(skb);
@@ -341,14 +344,22 @@ void netpoll_send_udp(struct netpoll *np
 
 static void arp_reply(struct sk_buff *skb)
 {
+	struct netpoll_info *npinfo = skb->dev->npinfo;
 	struct arphdr *arp;
 	unsigned char *arp_ptr;
 	int size, type = ARPOP_REPLY, ptype = ETH_P_ARP;
 	u32 sip, tip;
+	unsigned long flags;
 	struct sk_buff *send_skb;
-	struct netpoll *np = skb->dev->np;
+	struct netpoll *np = NULL;
+
+	spin_lock_irqsave(&npinfo->rx_lock, flags);
+	if (npinfo->rx_np && npinfo->rx_np->dev == skb->dev)
+		np = npinfo->rx_np;
+	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
 
-	if (!np) return;
+	if (!np)
+		return;
 
 	/* No arp on this interface */
 	if (skb->dev->flags & IFF_NOARP)
@@ -429,9 +440,9 @@ int __netpoll_rx(struct sk_buff *skb)
 	int proto, len, ulen;
 	struct iphdr *iph;
 	struct udphdr *uh;
-	struct netpoll *np = skb->dev->np;
+	struct netpoll *np = skb->dev->npinfo->rx_np;
 
-	if (!np->rx_hook)
+	if (!np)
 		goto out;
 	if (skb->dev->type != ARPHRD_ETHER)
 		goto out;
@@ -611,9 +622,8 @@ int netpoll_setup(struct netpoll *np)
 {
 	struct net_device *ndev = NULL;
 	struct in_device *in_dev;
-
-	np->poll_lock = SPIN_LOCK_UNLOCKED;
-	np->poll_owner = -1;
+	struct netpoll_info *npinfo;
+	unsigned long flags;
 
 	if (np->dev_name)
 		ndev = dev_get_by_name(np->dev_name);
@@ -624,7 +634,17 @@ int netpoll_setup(struct netpoll *np)
 	}
 
 	np->dev = ndev;
-	ndev->np = np;
+	if (!ndev->npinfo) {
+		npinfo = kmalloc(sizeof(*npinfo), GFP_KERNEL);
+		if (!npinfo)
+			goto release;
+
+		npinfo->rx_np = NULL;
+		npinfo->poll_lock = SPIN_LOCK_UNLOCKED;
+		npinfo->poll_owner = -1;
+		npinfo->rx_lock = SPIN_LOCK_UNLOCKED;
+	} else
+		npinfo = ndev->npinfo;
 
 	if (!ndev->poll_controller) {
 		printk(KERN_ERR "%s: %s doesn't support polling, aborting.\n",
@@ -692,13 +712,20 @@ int netpoll_setup(struct netpoll *np)
 		       np->name, HIPQUAD(np->local_ip));
 	}
 
-	if(np->rx_hook)
-		np->rx_flags = NETPOLL_RX_ENABLED;
+	if(np->rx_hook) {
+		spin_lock_irqsave(&npinfo->rx_lock, flags);
+		npinfo->rx_flags |= NETPOLL_RX_ENABLED;
+		npinfo->rx_np = np;
+		spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+	}
+	/* last thing to do is link it to the net device structure */
+	ndev->npinfo = npinfo;
 
 	return 0;
 
  release:
-	ndev->np = NULL;
+	if (!ndev->npinfo)
+		kfree(npinfo);
 	np->dev = NULL;
 	dev_put(ndev);
 	return -1;
@@ -706,9 +733,20 @@ int netpoll_setup(struct netpoll *np)
 
 void netpoll_cleanup(struct netpoll *np)
 {
-	if (np->dev)
-		np->dev->np = NULL;
-	dev_put(np->dev);
+	struct netpoll_info *npinfo;
+	unsigned long flags;
+
+	if (np->dev) {
+		npinfo = np->dev->npinfo;
+		if (npinfo && npinfo->rx_np == np) {
+			spin_lock_irqsave(&npinfo->rx_lock, flags);
+			npinfo->rx_np = NULL;
+			npinfo->rx_flags &= ~NETPOLL_RX_ENABLED;
+			spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+		}
+		dev_put(np->dev);
+	}
+
 	np->dev = NULL;
 }
 
--- linux-2.6.12-rc6/include/linux/netpoll.h.orig	2005-06-20 19:51:47.000000000 -0400
+++ linux-2.6.12-rc6/include/linux/netpoll.h	2005-06-21 15:29:48.000000000 -0400
@@ -16,14 +16,19 @@ struct netpoll;
 struct netpoll {
 	struct net_device *dev;
 	char dev_name[16], *name;
-	int rx_flags;
 	void (*rx_hook)(struct netpoll *, int, char *, int);
 	void (*drop)(struct sk_buff *skb);
 	u32 local_ip, remote_ip;
 	u16 local_port, remote_port;
 	unsigned char local_mac[6], remote_mac[6];
+};
+
+struct netpoll_info {
 	spinlock_t poll_lock;
 	int poll_owner;
+	int rx_flags;
+	spinlock_t rx_lock;
+	struct netpoll *rx_np; /* netpoll that registered an rx_hook */
 };
 
 void netpoll_poll(struct netpoll *np);
@@ -39,22 +44,35 @@ void netpoll_queue(struct sk_buff *skb);
 #ifdef CONFIG_NETPOLL
 static inline int netpoll_rx(struct sk_buff *skb)
 {
-	return skb->dev->np && skb->dev->np->rx_flags && __netpoll_rx(skb);
+	struct netpoll_info *npinfo = skb->dev->npinfo;
+	unsigned long flags;
+	int ret = 0;
+
+	if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
+		return 0;
+
+	spin_lock_irqsave(&npinfo->rx_lock, flags);
+	/* check rx_flags again with the lock held */
+	if (npinfo->rx_flags && __netpoll_rx(skb))
+		ret = 1;
+	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
+
+	return ret;
 }
 
 static inline void netpoll_poll_lock(struct net_device *dev)
 {
-	if (dev->np) {
-		spin_lock(&dev->np->poll_lock);
-		dev->np->poll_owner = smp_processor_id();
+	if (dev->npinfo) {
+		spin_lock(&dev->npinfo->poll_lock);
+		dev->npinfo->poll_owner = smp_processor_id();
 	}
 }
 
 static inline void netpoll_poll_unlock(struct net_device *dev)
 {
-	if (dev->np) {
-		spin_unlock(&dev->np->poll_lock);
-		dev->np->poll_owner = -1;
+	if (dev->npinfo) {
+		dev->npinfo->poll_owner = -1;
+		spin_unlock(&dev->npinfo->poll_lock);
 	}
 }
 
--- linux-2.6.12-rc6/include/linux/netdevice.h.orig	2005-06-20 20:26:21.000000000 -0400
+++ linux-2.6.12-rc6/include/linux/netdevice.h	2005-06-21 14:46:52.000000000 -0400
@@ -41,7 +41,7 @@
 struct divert_blk;
 struct vlan_group;
 struct ethtool_ops;
-struct netpoll;
+struct netpoll_info;
 					/* source back-compat hooks */
 #define SET_ETHTOOL_OPS(netdev,ops) \
 	( (netdev)->ethtool_ops = (ops) )
@@ -468,7 +468,7 @@ struct net_device
 						     unsigned char *haddr);
 	int			(*neigh_setup)(struct net_device *dev, struct neigh_parms *);
 #ifdef CONFIG_NETPOLL
-	struct netpoll		*np;
+	struct netpoll_info	*npinfo;
 #endif
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	void                    (*poll_controller)(struct net_device *dev);

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: David S. Miller @ 2005-06-22 20:43 UTC (permalink / raw)
  To: rick.jones2; +Cc: netdev, hadi, becker
In-Reply-To: <42B9CBA2.5050208@hp.com>

From: Rick Jones <rick.jones2@hp.com>
Date: Wed, 22 Jun 2005 13:35:46 -0700

> > Everyone seems to have this absolute fetish about batching the RX
> > descriptor refilling work.  It's wrong, it should be done when you
> > pull a receive packet off the ring, for many reasons.  Off the top of
> > my head:
> > 
> > 1) Descriptors are refilled as soon as possible, decreasing
> >    the chance of the device hitting the end of the RX ring
> >    and thus unable to receive a packet.
> 
> IFF one pokes the NIC for each buffer right?

Or "every 5" or something like that.

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Rick Jones @ 2005-06-22 20:35 UTC (permalink / raw)
  To: netdev; +Cc: hadi, becker
In-Reply-To: <20050622.132241.21929037.davem@davemloft.net>


> Everyone seems to have this absolute fetish about batching the RX
> descriptor refilling work.  It's wrong, it should be done when you
> pull a receive packet off the ring, for many reasons.  Off the top of
> my head:
> 
> 1) Descriptors are refilled as soon as possible, decreasing
>    the chance of the device hitting the end of the RX ring
>    and thus unable to receive a packet.

IFF one pokes the NIC for each buffer right?

rick jones

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: David S. Miller @ 2005-06-22 20:22 UTC (permalink / raw)
  To: ak; +Cc: leonid.grossman, hadi, becker, rick.jones2, netdev, davem
In-Reply-To: <20050622180654.GX14251@wotan.suse.de>

From: Andi Kleen <ak@suse.de>
Date: Wed, 22 Jun 2005 20:06:55 +0200

> However it is tricky because CPUs have only a limited load queue
> entries and doing too many prefetches will just overflow that.

Several processors can queue about 8 prefetch requests, and
these slots are independant of those consumed by a load.

Yes, if you queue too many prefetches, the queue overflows.

I think the optimal scheme would be:

1) eth_type_trans() info in RX descriptor
2) prefetch(skb->data) done as early as possible in driver
   RX handling

Actually, I believe to most optimal scheme is:

foo_driver_rx()
{
	for_each_rx_descriptor() {
		...
		skb = driver_priv->rx_skbs[index];
		prefetch(skb->data);

		skb = realloc_or_recycle_rx_descriptor(skb, index);
		if (skb == NULL)
			goto next_rxd;

		skb->prot = eth_type_trans(skb, driver_priv->dev);
		netif_receive_skb(skb);
		...
	next_rxd:
		...
	}
}

The idea is that first the prefetch goes into flight, then you do the
recycle or reallocation of the RX descriptor SKB, then you try to
touch the data.

This makes it very likely the prefetch will be in the cpu in time.

Everyone seems to have this absolute fetish about batching the RX
descriptor refilling work.  It's wrong, it should be done when you
pull a receive packet off the ring, for many reasons.  Off the top of
my head:

1) Descriptors are refilled as soon as possible, decreasing
   the chance of the device hitting the end of the RX ring
   and thus unable to receive a packet.

2) As shown above, it gives you compute time which can be used to
   schedule the prefetch.  This nearly makes RX replenishment free.
   Instead of having the CPU spin on a cache miss when we run
   eth_type_trans() during those cycles, we do useful work.

I'm going to play around with these ideas in the tg3 driver.
Obvious patch below.

--- 1/drivers/net/tg3.c.~1~	2005-06-22 12:33:07.000000000 -0700
+++ 2/drivers/net/tg3.c	2005-06-22 13:19:13.000000000 -0700
@@ -2772,6 +2772,13 @@
 			goto next_pkt_nopost;
 		}
 
+		/* Prefetch now.  The recycle/realloc of the RX
+		 * entry is moderately expensive, so by the time
+		 * that is complete the data should have reached
+		 * the cpu.
+		 */
+		prefetch(skb->data);
+
 		work_mask |= opaque_key;
 
 		if ((desc->err_vlan & RXD_ERR_MASK) != 0 &&

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: jamal @ 2005-06-22 19:37 UTC (permalink / raw)
  To: P
  Cc: David S. Miller, gandalf, shemminger, mitch.a.williams,
	john.ronciak, mchan, buytenh, jdmason, netdev, Robert.Olsson,
	ganesh.venkatesan, jesse.brandeburg
In-Reply-To: <42B92490.40005@draigBrady.com>

On Wed, 2005-22-06 at 09:42 +0100, P@draigBrady.com wrote:

> 
> Yes the copy is essentially free here as the data is already cached.
> 
> As a data point, I went the whole hog and used buffer recycling
> in my essentially packet sniffing application. I.E. there are no
> allocs per packet at all, and this make a HUGE difference. On a
> 2x3.4GHz 2xe1000 system I can receive 620Kpps per port sustained
> into my userspace app which does a LOT of processing per packet.
> Without the buffer recycling is was around 250Kpps.
> Note I don't reuse an skb until the packet is copied into a
> PACKET_MMAP buffer.

Was this machine SMP? NAPI involved? I take it nothing interfering in
the middle with the headers?

cheers,
jamal

^ permalink raw reply

* Re: [TG3]: About hw coalescing infrastructure.
From: Michael Chan @ 2005-06-22 19:03 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David S. Miller, netdev
In-Reply-To: <42B982D4.9040704@cosmosbay.com>

On Wed, 2005-06-22 at 17:25 +0200, Eric Dumazet wrote:

> Is there anything I can try to tune the coalescing ?
> Being able to handle 100 packets each interrupt instead of one or two would certainly help.
> I dont mind about latency. But of course I would like not to drop packets :)
> But maybe the BCM5702 is not able to delay an interrupt ?
> 

On the 5702 that supports CLRTCKS mode, you need to play around with the
following parameters in tg3.h. To reduce interrupts, you generally have
to increase the values.

#define  LOW_RXCOL_TICKS_CLRTCKS	 0x00000014
#define  LOW_TXCOL_TICKS_CLRTCKS	 0x00000048
#define  LOW_RXMAX_FRAMES		 0x00000005
#define  LOW_TXMAX_FRAMES		 0x00000035
#define  DEFAULT_RXCOAL_TICK_INT_CLRTCKS 0x00000014
#define  DEFAULT_TXCOAL_TICK_INT_CLRTCKS 0x00000014
#define  DEFAULT_RXCOAL_MAXF_INT	 0x00000005
#define  DEFAULT_TXCOAL_MAXF_INT	 0x00000005

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-22 18:06 UTC (permalink / raw)
  To: Leonid Grossman
  Cc: hadi, 'Donald Becker', 'Andi Kleen',
	'Rick Jones', netdev, davem
In-Reply-To: <200506221801.j5MI11xS021866@guinness.s2io.com>

> Outside of the filters capability, from the (granted, pretty limited)
> testing we see some noticeable improvement from providing status bits but it
> is not as big as I would expect,
> It looks like the headers are still being touched somewhere... We will look
> at this some more.

The headers are read of course in the main stack. No way around that.

It basically helps you only when you can space the prefetch for the header
out long enough that the data is in cache when you need it. However
it is tricky because CPUs have only a limited load queue entries and doing
too many prefetches will just overflow that.

This can be done by batching L2 packet processing, but doing so 
is not good for your latency.

-Andi

^ permalink raw reply

* RE: RFC: NAPI packet weighting patch
From: Leonid Grossman @ 2005-06-22 18:00 UTC (permalink / raw)
  To: hadi
  Cc: 'Donald Becker', 'Andi Kleen',
	'Rick Jones', netdev, davem
In-Reply-To: <1119458226.6918.142.camel@localhost.localdomain>

 

> -----Original Message-----
> From: jamal [mailto:hadi@cyberus.ca] 
> Sent: Wednesday, June 22, 2005 9:37 AM
> To: Leonid Grossman
> Cc: 'Donald Becker'; 'Andi Kleen'; 'Rick Jones'; 
> netdev@oss.sgi.com; davem@redhat.com
> Subject: RE: RFC: NAPI packet weighting patch
> 
> On Wed, 2005-22-06 at 09:23 -0700, Leonid Grossman wrote:
> > > 
> > > See the comment above. We decide if a packet is multicast vs. 
> > > unicast, IP vs. other at approximately interrupt/"rx_copybreak" 
> > > time.  Very few NIC provide this info in status bits, so 
> we end up 
> > > looking at the packet header.  That read moves the previously 
> > > known-uncached data (after all, it was just came in from a bus 
> > > write) into the L1 cache for the CPU handling the device. 
>  Once it's 
> > > there, the copy is almost free.
> > 
> > What status bits a NIC has to provide, in order for the 
> stack to avoid 
> > touching headers?
> > In our case, the headers are separated by the hardware so 
> ideally we 
> > would like to avoid any header processing altogether, and 
> reduce the 
> > number of cache misses.
> > 
> 
> Provide metadata that can be used to totaly replace 
> eth_type_trans() i.e answer the questions: is it 
> multi/uni/broadcast, is the packet for us (you would need to 
> be programmed with what for us means), Is it IP, ARP etc. I 
> am sure any standard NIC these days can do a subset of these  
> You want to go one step further then allow the user to 
> download a number of filters and tell you what tag you should 
> put on the descriptor when sending the packet to user space 
> on a match or mismatch. 
> If say you allowed 1024 such filters (not very different from 
> the current multicast filters), you could cut down a lot of CPU time.


Well, this is all supported in the hardware. 
The number of filters is only 256 (not 1024) for direct match, but it is
unlimited for a hash match.
Of course, the upper layer still needs to be able to take advantage of the
filters...

Outside of the filters capability, from the (granted, pretty limited)
testing we see some noticeable improvement from providing status bits but it
is not as big as I would expect,
It looks like the headers are still being touched somewhere... We will look
at this some more.

Thanks, Leonid

> 
> cheers,
> jamal
> 
> 

^ permalink raw reply

* Re: RFC: NAPI packet weighting patch
From: Andi Kleen @ 2005-06-22 17:05 UTC (permalink / raw)
  To: Leonid Grossman
  Cc: 'Donald Becker', 'Andi Kleen',
	'Rick Jones', netdev, davem
In-Reply-To: <200506221623.j5MGNkxS001340@guinness.s2io.com>

On Wed, Jun 22, 2005 at 09:23:41AM -0700, Leonid Grossman wrote:
> 
> > 
> > See the comment above. We decide if a packet is multicast vs. 
> > unicast, IP vs. other at approximately 
> > interrupt/"rx_copybreak" time.  Very few NIC provide this 
> > info in status bits, so we end up looking at the packet 
> > header.  That read moves the previously known-uncached data 
> > (after all, it was just came in from a bus write) into the L1 
> > cache for the CPU handling the device.  Once it's there, the 
> > copy is almost free.
> 
> What status bits a NIC has to provide, in order for the stack to avoid
> touching headers?

To avoid it completely is pretty hard - you would need to supply
nearly everything in the header.

But when you supply L2 protocol/ and unicast/broadcast/multicast information
and if the packet is destined to the localhost or not then the 
headers can be gotten with a prefetch early and then when 
the header is later processed then it might be with some luck
already in cache.

BTW quite a few modern NICs provide this information actually contrary
to what Donald stated (sometimes with restrictions like only
works without multicast), but it hasn't been widely used yet.

-Andi

^ permalink raw reply

* Re: [patch,rfc] allow registration of multiple netpolls per interface
From: Matt Mackall @ 2005-06-22 17:01 UTC (permalink / raw)
  To: Jeff Moyer; +Cc: netdev, netdev, linux-kernel
In-Reply-To: <17081.20441.714191.528270@segfault.boston.redhat.com>

On Wed, Jun 22, 2005 at 07:47:37AM -0400, Jeff Moyer wrote:
> mpm> Hmm. It's conceivable we'll want netdump and kgdb on the same
> mpm> interface so we'll have to address this eventually..
> 
> Well, do you want to address it eventually, or now?  As I said, it's never
> bitten anyone before.

Later's fine. I just don't want to design it out by accident again.
 
> >> struct netpoll_info {
> >> spinlock_t poll_lock;
> >> int poll_owner;
> >> int rx_flags;
> >> spinlock_t rx_lock;
> >> struct netpoll *rx_np; /* netpoll that registered an rx_hook */
> >> };
> >> 
> >> This is the structure which gets pointed to by the net_device.  All of the
> >> flags and locks which are specific to the INTERFACE go here.  Any variables
> >> which must be kept per struct netpoll were left in the struct netpoll.  So
> >> now, we have a cleaner separation of data and its scope.
> >> 
> >> Since we never really supported having more than one struct netpoll
> >> register an rx_hook, I got rid of the rx_list.  This is replaced by a
> >> single pointer in the netpoll_info structure (np_rx).  We still need to
> >> protect addition or removal of the rx_np pointer, and so keep the lock
> >> (rx_lock).  There is one lock per struct net_device, and I am certain that
> >> it will be 0 contention, as rx_np will only be changed during an insmod or
> >> rmmod.  If people think this would be a good rcu candidate, let me know and
> >> I'll change it to use that locking scheme.
> 
> mpm> It might be simpler to have a single lock here..?
> 
> Maybe.  You can't really have netpoll code running on multiple cpus at the
> same time, right?  This is the rx path, remember, so the other cpu should
> be spinning on the poll_lock.
> 
> Keeping separate locks would allow you to unregister a struct netpoll
> associated with another net device without causing lock contention.  This
> is a very minor win, obviously.
> 
> I still feel like this npinfo struct is the right place for this, though.
> If you're strongly opposed to that, I'll change it.

No, certainly having it in npinfo makes sense. I just was wondering if
we really need two locks in there.

> >> +	spin_lock_irqsave(&npinfo->rx_lock, flags);
> >> +	if (npinfo->rx_np->dev == skb->dev)
> >> +		np = npinfo->rx_np;
> >> +	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
> 
> mpm> And I think that means we don't need the lock here either.  
> 
> Sure we do.  We need to protect against rmmod's.

How can we have an rmmmod when we're trapped?

> >> static inline int netpoll_rx(struct sk_buff *skb)
> >> {
> >> -	return skb->dev->np && skb->dev->np->rx_flags && __netpoll_rx(skb);
> >> +	struct netpoll_info *npinfo = skb->dev->npinfo;
> >> +	unsigned long flags;
> >> +	int ret = 0;
> >> +
> >> +	if (!npinfo || (!npinfo->rx_np && !npinfo->rx_flags))
> >> +		return 0;
> >> +
> >> +	spin_lock_irqsave(&npinfo->rx_lock, flags);
> >> +	/* check rx_flags again with the lock held */
> >> +	if (npinfo->rx_flags && __netpoll_rx(skb))
> >> +		ret = 1;
> >> +	spin_unlock_irqrestore(&npinfo->rx_lock, flags);
> >> +
> >> +	return ret;
> >> }
> 
> mpm> This is perhaps a problem due to cache line bouncing. Perhaps we can
> mpm> use an atomic op and a memory barrier instead?
> 
> It really should be a 0 contention lock.  Let's not optimize something that
> doesn't need it.  If we find that it causes problems, I'll be more than
> happy to fix it.

Ok, fair enough.

-- 
Mathematics is the supreme nostalgia of our time.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox