Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH 2.6.23-rc6] NETWORKING : Edge Triggered EPOLLOUT events get missed for TCP sockets
From: David Miller @ 2007-09-19 16:02 UTC (permalink / raw)
  To: tomer_iisc; +Cc: netdev, linux-kernel
In-Reply-To: <106538.13069.qm@web53707.mail.re2.yahoo.com>

From: Nagendra Tomar <tomer_iisc@yahoo.com>
Date: Wed, 19 Sep 2007 04:13:24 -0700 (PDT)

> With the SOCK_NOSPACE check in tcp_check_space(), this epoll_wait call will 
> not return, even when the incoming acks free the buffers.
>         Note that this patch assumes that the SOCK_NOSPACE check in
> tcp_check_space is a trivial optimization which can be safely removed.

I think the more correct fix is to fix whoever isn't
setting the SOCK_NOSPACE bit, that check really is
crucial.

^ permalink raw reply

* [ofa-general] Re: [Bug, PATCH and another Bug] Was: Fix refcounting problem with netif_rx_reschedule()
From: David Miller @ 2007-09-19 16:05 UTC (permalink / raw)
  To: krkumar2; +Cc: netdev, rdreier, general
In-Reply-To: <20070919115403.19455.65941.sendpatchset@K50wks273871wss.in.ibm.com>

From: Krishna Kumar <krkumar2@in.ibm.com>
Date: Wed, 19 Sep 2007 17:24:03 +0530

> Note: during steps F-H and C-E, priv/napi is read/modified by both cpu's
> 	which is another bug relating to the same race.
> 
> I guess the above patch is not required if this bug (in IPoIB) is fixed?

The NAPI_STATE_SCHED flag bit should provide all of the necessary
synchornization.

Only the setter of that bit should add the NAPI instance to the
polling list.

The polling loop runs atomically on the cpu where the NAPI instance
got added to the per-cpu polling list.  And therefore decisions to
complete NAPI are serialized too.

That serialized completion decision is also when the list deletion
occurs.

I'm starting to suspect the whole problem comes from the resched
facility, and now I really don't blame Stephen for trying to delete
it.  Semantically it really makes things very difficult, especially
wrt. to the atomicity of the list handling.

^ permalink raw reply

* Re: [RFC][NET_SCHED] explict hold dev tx lock
From: David Miller @ 2007-09-19 16:09 UTC (permalink / raw)
  To: hadi; +Cc: herbert, netdev, kaber, dada1, johnpol
In-Reply-To: <1190208832.4259.6.camel@localhost>

From: jamal <hadi@cyberus.ca>
Date: Wed, 19 Sep 2007 09:33:52 -0400

> On Mon, 2007-17-09 at 22:48 -0400, jamal wrote:
> 
> > Nothing much has changed from what it was before.
> > The only difference is we let go of the queue lock before grabbing
> > the tx lock which never mattered for LLTX. 
> > Once we grab the tx lock it is the same logic and so far is working well
> > on both tg3 and e1000 (which is LLTX). 
> > I will continue to retest with net-2.6.24 once you complete rebasing
> > and look around to see if anyone maybe affected.
> 
> Ok, this is looking solid with this mornings tree. Tested on a dual core
> xeon with e1000 (LLTX) and a dual core opteron with tg3 (non-LLTX).
> About 100 million packets from udp full throttle on all 4 cpus; i tried
> pulling cables etc while doing this to generate extrenous interupts and
> didnt see any issues.
> 
> Shall i submit the patch?

Sure, along with a description as to why you want to make this
change.

I still don't understand the impetus. :)

^ permalink raw reply

* Re: bnx2 dirver's firmware images
From: David Miller @ 2007-09-19 16:09 UTC (permalink / raw)
  To: davidsen; +Cc: mchan, vda.linux, linux-kernel, netdev
In-Reply-To: <46F126BE.5060507@tmr.com>

From: Bill Davidsen <davidsen@tmr.com>
Date: Wed, 19 Sep 2007 09:40:14 -0400

> Is that a suggestion that the driver work differently when built as a 
> module or built in?

Absolutely.


^ permalink raw reply

* Re: follow-up: discrepancy with POSIX
From: David Miller @ 2007-09-19 16:15 UTC (permalink / raw)
  To: drepper; +Cc: netdev, linux-kernel
In-Reply-To: <46F13E8B.4050309@redhat.com>

From: Ulrich Drepper <drepper@redhat.com>
Date: Wed, 19 Sep 2007 08:21:47 -0700

> If you think the POSIX spec is wrong (and can point to other
> implementations doing the same as Linux) let me know and I'll work on
> getting the spec changed.

The whole AF_UNSPEC thing I'm almost certain comes from BSD, which has
behaved that way for centuries.

Someone needs to cull through Steven's Volume 2 to verify this, I'm
too busy at the moment to do so myself.

^ permalink raw reply

* Re: bnx2 dirver's firmware images
From: maximilian attems @ 2007-09-19 16:33 UTC (permalink / raw)
  To: Michael Chan; +Cc: David Miller, vda.linux, linux-kernel, netdev
In-Reply-To: <1190145951.9540.230.camel@dell>

On Tue, 18 Sep 2007, Michael Chan wrote:

> The bnx2 firmware changes quite frequently.  A new driver quite often
> requires new firmware to work correctly.  Splitting them up makes things
> difficult for the user.

sorry didn't check, what's the license of the firmware?
 
> The firmware in tg3 is a lot more mature and I don't expect it to
> change.  I think tg3 is better suited for using request_firmware().

well thanks that would help us a lot,
we have to strip the firmware for Debian for the upcoming Lenny release.
as the shipping exception for the previous 2 releases will not be granted
again. so a separate firmware would be great. also afaik only some boards
need it. it is a pressing need for us as tg3 with stripped firmware of
course doesn't build.

-- 
maks

^ permalink raw reply

* Re: bnx2 dirver's firmware images
From: David Miller @ 2007-09-19 16:38 UTC (permalink / raw)
  To: max; +Cc: mchan, vda.linux, linux-kernel, netdev
In-Reply-To: <20070919163318.GG6618@stro.at>

From: maximilian attems <max@stro.at>
Date: Wed, 19 Sep 2007 18:33:18 +0200

> we have to strip the firmware for Debian for the upcoming Lenny release.

Why do you "have to"?  The vendor has given you explicit rights
to distribute it:

 * Firmware is:
 *	Derived from proprietary unpublished source code,
 *	Copyright (C) 2000-2003 Broadcom Corporation.
 *
 *	Permission is hereby granted for the distribution of this firmware
 *	data in hexadecimal or equivalent format, provided this copyright
 *	notice is accompanying it.

This whole firmware stripping thing in debian is beyond rediculious
and only serves to hurt users.

^ permalink raw reply

* Re: follow-up: discrepancy with POSIX
From: Alan Cox @ 2007-09-19 16:39 UTC (permalink / raw)
  To: David Miller; +Cc: drepper, netdev, linux-kernel
In-Reply-To: <20070919.091510.00787167.davem@davemloft.net>

On Wed, 19 Sep 2007 09:15:10 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Ulrich Drepper <drepper@redhat.com>
> Date: Wed, 19 Sep 2007 08:21:47 -0700
> 
> > If you think the POSIX spec is wrong (and can point to other
> > implementations doing the same as Linux) let me know and I'll work on
> > getting the spec changed.
> 
> The whole AF_UNSPEC thing I'm almost certain comes from BSD, which has
> behaved that way for centuries.

We got it from the 1003.4g draft socket specification if I remember
rightly. Its entirely plausible that got it from 4BSE.

Alan

^ permalink raw reply

* Re: [ofa-general] Re: [PATCH 02/11] IB/ipoib: Notify the world before doing unregister
From: Moni Shoua @ 2007-09-19 16:41 UTC (permalink / raw)
  To: Roland Dreier, Jay Vosburgh; +Cc: netdev, jgarzik, davem, general
In-Reply-To: <ada3axblvj2.fsf@cisco.com>


Roland, Jay, 
Thanks a lot for the comments.
I'd like to summarize the points raised so far

1. Reduce the indentation in patch #4 (Roland)
	I will resend
2. Remove the "if (n->dev->flags & IFF_MASTER)" from patch #3 (Roland)
	I will resend
3. Consider making ipoib_slave_detach() net/core/dev.c (Roland, Jay)
	I think that this is a good idea. I can make the patch (and necessary changes to 
	the other patches) assuming this is agreed by all.
4. Change header for patch #1 (Roland)
	I will resend
5. Use NETDEV_GOING_DOWN and not NETDEV_CHANGE + IFF_SLAVE_DETACH (Jay)
	The  NETDEV_GOING_DOWN event is sent in the contex of unregister_netdevice() 
	Since the action in bonding to the event should be unregister the bonding master
	it is not possible to do so. bonding needs to know about the slave detach earlier.
6. call notifiers from unregister_netdev()
	See answer to 5.
7. missing call to notifiers in ipoib_vlan_delete() (Roland)
	It seems like you're right. I will fix and resend.

I think that if there are no other comments, I will submit the entire 11 patches 
again (with changes) to make it easier to merge into the kernel. Since the most of the 
content in the patch series is in bonding I thought it would be right that Jay will 
push all the patches to the networking git. Is it OK with you Roland?




 

^ permalink raw reply

* Re: [ofa-general] Re: [PATCH 02/11] IB/ipoib: Notify the world before doing unregister
From: Roland Dreier @ 2007-09-19 16:44 UTC (permalink / raw)
  To: Moni Shoua; +Cc: Jay Vosburgh, netdev, jgarzik, davem, general
In-Reply-To: <46F15155.4070708@voltaire.com>

 > I think that if there are no other comments, I will submit the entire 11 patches 
 > again (with changes) to make it easier to merge into the kernel. Since the most of the 
 > content in the patch series is in bonding I thought it would be right that Jay will 
 > push all the patches to the networking git. Is it OK with you Roland?

Yes, that's fine.

 - R.

^ permalink raw reply

* Re: [PATCH net-2.6.24] eHEA: poll function update for new NAPI scheme
From: David Miller @ 2007-09-19 16:47 UTC (permalink / raw)
  To: ossthema; +Cc: netdev, raisch, themann, linux-kernel, linuxppc-dev, meder,
	tklein
In-Reply-To: <200709191740.30640.ossthema@de.ibm.com>

From: Jan-Bernd Themann <ossthema@de.ibm.com>
Date: Wed, 19 Sep 2007 17:40:30 +0200

> Update of ehea_poll function to work with new NAPI scheme.
> 
> Signed-off-by: Jan-Bernd Themann <themann@de.ibm.com>

Applied, thanks.

^ permalink raw reply

* Re: [NET-2.6.24][VETH][patch 1/1] fix bad netif_carrier_off place
From: David Miller @ 2007-09-19 16:48 UTC (permalink / raw)
  To: dlezcano; +Cc: xemul, netdev
In-Reply-To: <20070919140850.607582917@mai.toulouse-stg.fr.ibm.com>

From: dlezcano@fr.ibm.com
Date: Wed, 19 Sep 2007 15:38:22 +0200

> From: Daniel Lezcano <dlezcano@fr.ibm.com>
> 
> If the netif_carrier_off is called before register_netdev
> that will use and generate an event for a non initialized network
> device and that leads to a Oops.
> 
> I moved the netif_carrier_off from the setup function after each 
> register_netdev call.
> 
> Signed-off-by: Daniel Lezcano <dlezcano@fr.ibm.com>

Applied, thanks.

^ permalink raw reply

* Re: follow-up: discrepancy with POSIX
From: Ulrich Drepper @ 2007-09-19 16:49 UTC (permalink / raw)
  To: Andi Kleen; +Cc: netdev, Linux Kernel
In-Reply-To: <p73sl5aveqh.fsf@bingen.suse.de>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andi Kleen wrote:
> The standard way to undo connect is to use AF_UNSPEC. Code to handle
> that for dgram sockets is there. It's the same code for v4 and v6.

I quoted the standard and it does not say anything about AF_UNSPEC.  So
you cannot simply make such broad statements.

I also don't say that this behavior should be removed.  It's certainly
useful, very much so in fact.

But the spec calls for a "null address" to be used and that's in my
understanding something different from using AF_UNSPEC.

I looked through Stevens TCP Illustrated Vol 2 and it seems not to
mention resetting the address at all.  The POSIX spec certainly got this
text from .1g.

I cannot test it on other systems.  If somebody has access to some
certified systems (and maybe others), write a bit of code which creates
a DGRAM socket, connect to one address, call connect with a "null
address", then connect to another address (which likely has to use a
different interface since otherwise the connect will just succeed, it
seems).

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFG8VMF2ijCOnn/RHQRAr9NAJwLxyql0kQnMGJNaPZlRGsuB6rGEACgog88
WIWAFhuBWsjps7PdbcoumUQ=
=oLxP
-----END PGP SIGNATURE-----

^ permalink raw reply

* Re: bnx2 dirver's firmware images
From: maximilian attems @ 2007-09-19 16:51 UTC (permalink / raw)
  To: David Miller; +Cc: mchan, vda.linux, linux-kernel, netdev
In-Reply-To: <20070919.093832.88002774.davem@davemloft.net>

hello dave,

i appreciate a lot your opinon, but please cool down.
this is not a four spin on your beloved pipe. :)

On Wed, Sep 19, 2007 at 09:38:32AM -0700, David Miller wrote:
> From: maximilian attems <max@stro.at>
> Date: Wed, 19 Sep 2007 18:33:18 +0200
> 
> > we have to strip the firmware for Debian for the upcoming Lenny release.
> 
> Why do you "have to"?  The vendor has given you explicit rights
> to distribute it:
> 
>  * Firmware is:
>  *	Derived from proprietary unpublished source code,
>  *	Copyright (C) 2000-2003 Broadcom Corporation.
>  *
>  *	Permission is hereby granted for the distribution of this firmware
>  *	data in hexadecimal or equivalent format, provided this copyright
>  *	notice is accompanying it.

afair the trouble is that it does not give permission to change
unlike some other gpl or bsd licensed blob.
so it is dfsg non-free and not suitable for main distribution.
 
> This whole firmware stripping thing in debian is beyond rediculious
> and only serves to hurt users.

i'm not of the d-legal department,
but seeing free firmwares would be cool.

-- 
maks


^ permalink raw reply

* Re: [ofa-general] [PATCH] RDMA/CMA: Implement rdma_resolve_ip retry enhancement.
From: Sean Hefty @ 2007-09-19 16:52 UTC (permalink / raw)
  To: ggrundstrom; +Cc: rdreier, netdev, general
In-Reply-To: <200709190022.l8J0MbWt024754@neteffect.com>

> If an application is calling rdma_resolve_ip() and a status of -ENODATA is returned from addr_resolve_local/remote(), the timeout mechanism waits until the application's timeout occurs before rechecking the address resolution status; the application will wait until it's full timeout occurs.  This case is seen when the work thread call to process_req() is made before the arp packet is processed.

I don't understand the issue.  process_req() is invoked whenever a 
network event occurs, which rechecks all pending requests.

> This patch is in addition to Steve Wise's neigh_event_send patch to initiate neighbour discovery sent on 9/12/2007.

This patch looks unrelated to Steve's patch.  Can you clarify the 
relationship?

- Sean

^ permalink raw reply

* Re: follow-up: discrepancy with POSIX
From: David Miller @ 2007-09-19 16:52 UTC (permalink / raw)
  To: drepper; +Cc: andi, netdev, linux-kernel
In-Reply-To: <46F15305.2030507@redhat.com>

From: Ulrich Drepper <drepper@redhat.com>
Date: Wed, 19 Sep 2007 09:49:09 -0700

> But the spec calls for a "null address" to be used and that's in my
> understanding something different from using AF_UNSPEC.

It just occured to me that AF_UNSPEC might be used simply
because "all zeros" might be a valid real bindable address
for some address family.  And using AF_UNSPEC avoids that
problem entirely.

^ permalink raw reply

* new NAPI quota synchronization issue
From: David Miller @ 2007-09-19 16:58 UTC (permalink / raw)
  To: netdev; +Cc: shemminger


Ok, as has been hinted at with some postings from Krishna and others,
we still have some mutual exclusion issues in the new NAPI code.

In short, the napi->quota updates happen outside of the sections where
the code stream "owns" the napi_struct instance, so it can be modified
in parallel on multiple cpus, the n->quota can go negative, and the
quota bug checks trigger.

It just seems that gradually I'm reverting every single cleanup done
by Stephen in his original patch, first the list handling and now the
quota bits too :-)

Probably a good way to deal with this is to simply make the quota
handling stateless.

The only reason we handle partial quota usage is to handle the
global netdev_budget.  But we can simply "round up" and let
netdev_budget get oversubscribed by one napi->quota's worth
if necessary.

At that point, n->quota only holds two states when used, full
and empty.  And at that point there is no reason to maintain
it's value at all.  Only the weight is necessary.

I'll try to post a patch which implements this later today.

^ permalink raw reply

* Re: bnx2 dirver's firmware images
From: maximilian attems @ 2007-09-19 17:10 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: David Miller, hpa, mchan, vda.linux, linux-kernel, netdev
In-Reply-To: <20070918203530.GC27836@uranus.ravnborg.org>

On Tue, 18 Sep 2007, Sam Ravnborg wrote:

> Anyway - if we again consider klibc I will do my best to make the
> build stuff as smooth as possible.
> 
> 	Sam

<nitpicking mode on>
currently klibc has tendency to rebuild a bit too much if you
touch some part of it, seen in usr/utils
for the main klibc i agree that it makes sense

-- 
maks

^ permalink raw reply

* Re: bnx2 dirver's firmware images
From: H. Peter Anvin @ 2007-09-19 17:12 UTC (permalink / raw)
  To: maximilian attems
  Cc: Sam Ravnborg, David Miller, mchan, vda.linux, linux-kernel,
	netdev
In-Reply-To: <20070919171033.GH6618@stro.at>

maximilian attems wrote:
> On Tue, 18 Sep 2007, Sam Ravnborg wrote:
> 
>> Anyway - if we again consider klibc I will do my best to make the
>> build stuff as smooth as possible.
>>
>> 	Sam
> 
> <nitpicking mode on>
> currently klibc has tendency to rebuild a bit too much if you
> touch some part of it, seen in usr/utils
> for the main klibc i agree that it makes sense
> 

Could you give a concrete example?

	-hpa

^ permalink raw reply

* Re: bnx2 dirver's firmware images
From: maximilian attems @ 2007-09-19 17:18 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Sam Ravnborg, David Miller, mchan, vda.linux, linux-kernel,
	netdev
In-Reply-To: <46F15886.6040701@zytor.com>

On Wed, Sep 19, 2007 at 10:12:38AM -0700, H. Peter Anvin wrote:
> 
> Could you give a concrete example?
> 
> 	-hpa

max@dual:~/src/klibc$ touch usr/utils/mount_main.c
max@dual:~/src/klibc$ make
  KLIBCCC usr/utils/mount_main.o
  KLIBCLD usr/utils/static/halt
  LN      usr/utils/static/reboot
  LN      usr/utils/static/poweroff
  KLIBCLD usr/utils/shared/halt
  LN      usr/utils/shared/reboot
  LN      usr/utils/shared/poweroff
  KLIBCLD usr/utils/static/chroot
  KLIBCLD usr/utils/static/dd
  <snipp>

-- 
maks

^ permalink raw reply

* Re: follow-up: discrepancy with POSIX
From: Andi Kleen @ 2007-09-19 17:26 UTC (permalink / raw)
  To: Ulrich Drepper; +Cc: Andi Kleen, netdev, Linux Kernel
In-Reply-To: <46F15305.2030507@redhat.com>

On Wed, Sep 19, 2007 at 09:49:09AM -0700, Ulrich Drepper wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Andi Kleen wrote:
> > The standard way to undo connect is to use AF_UNSPEC. Code to handle
> > that for dgram sockets is there. It's the same code for v4 and v6.
> 
> I quoted the standard and it does not say anything about AF_UNSPEC.  So
> you cannot simply make such broad statements.

Ok "standard" was perhaps a poor choice of words.

AF_UNSPEC used to be introduced long ago by Alan based on some early 
POSIX draft iirc.

Also incidentially it's a null address:

include/linux/socket.h:#define AF_UNSPEC        0

> But the spec calls for a "null address" to be used and that's in my
> understanding something different from using AF_UNSPEC.

memset(&sockaddr, 0, sizeof(sockaddr)) should give you AF_UNSPEC

-Andi

^ permalink raw reply

* Re: follow-up: discrepancy with POSIX
From: Ulrich Drepper @ 2007-09-19 17:27 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <46F156A7.5000304@redhat.com>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ulrich Drepper wrote:
> Yes, but for IPv4/6 it's not an issue.  Some implementations might
> handle all-zeros and the spec _currently_ calls for it.  In this case an
> alignment would be good.

Searching the web shows up this:

http://developer.apple.com/documentation/Darwin/Reference/ManPages/man2/connect.2.html


  Datagram sockets may dissolve the association by connecting to an
  invalid address, such as a null address or an address with the address
  family set to AF_UNSPEC (the error EAFNOSUPPORT will be harmlessly
  returned).


I.e., at least Apple implements both variants.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (GNU/Linux)

iD8DBQFG8Vvu2ijCOnn/RHQRAsSfAJkBELtiNyul8wMOjVv1x7LfvDWw/ACfR0D0
cm+k1wfhCsT4GjbF3uac+eY=
=nksN
-----END PGP SIGNATURE-----

^ permalink raw reply

* Re: new NAPI quota synchronization issue
From: David Miller @ 2007-09-19 17:35 UTC (permalink / raw)
  To: netdev; +Cc: shemminger
In-Reply-To: <20070919.095825.39465358.davem@davemloft.net>

From: David Miller <davem@davemloft.net>
Date: Wed, 19 Sep 2007 09:58:25 -0700 (PDT)

> Probably a good way to deal with this is to simply make the quota
> handling stateless.
> 
> The only reason we handle partial quota usage is to handle the
> global netdev_budget.  But we can simply "round up" and let
> netdev_budget get oversubscribed by one napi->quota's worth
> if necessary.
> 
> At that point, n->quota only holds two states when used, full
> and empty.  And at that point there is no reason to maintain
> it's value at all.  Only the weight is necessary.
> 
> I'll try to post a patch which implements this later today.

Ok, here is the patch and I've checked it into net-2.6.24 as well.

There really shouldn't be any fundamental synchronization issues
in the new NAPI stuff any longer.  I'm pretty sure any problems
remaining can only be caused by drivers bugs but we'll see :-)

I went over the list handling several times and it looks bulletproof.
Only the thread of control that sets the NAPI_STATE_SCHED bit
atomically will do list modifications, until the thread of control
that decides unconditionally to clear the bit, which will do a
list_del() immediately before clearing that bit.

commit d97459caa5dc97b5da0b9be1ec3f107f3c58d7f9
Author: David S. Miller <davem@sunset.davemloft.net>
Date:   Wed Sep 19 10:31:58 2007 -0700

    [NAPI]: Make quota management stateless.
    
    Because we update the napi->quota after returning from
    napi->poll() we have races which can, among other things,
    allow napi->quota to go negative.
    
    For example, if the driver uses the NAPI resched mechanism
    it typically does a completion like this:
    
    	netif_rx_complete(dev, napi);
    	if (unlikely(more_work_showed_up(priv))) {
    		if (netif_rx_reschedule(dev, napi))
    			goto poll_more;
    	}
    	return work_done;
    
    Between the netif_rx_complete() and the netif_rx_reschedule()
    an interrupt on another cpu can schedule the NAPI.  Which is
    fine and handled by the checking of the netif_rx_reschedule()
    return value, but when it happens:
    
    1) The other cpu can do a rull ->poll() run, and update the
       napi->quota
    2) The current thread of execution returns and updates
       napi->quota too
    
    So #1 uses a not-updated napi->quota value, and #2 over
    subtracts from napi->quota.
    
    In short napi->quota access is not synchronized enough.
    
    The good news it that we don't really need it in the first
    place.  The only time we can have partial napi->quota
    updates is when we are trying to adhere to netdev_budget
    in the polling loop of net_rx_action().
    
    We can allow a slight oversubscription of netdev_budget,
    but at most one napi->weight, and that is harmless.
    
    Given that, napi->quota takes on only two values when used,
    the full napi->weight and zero.  Therefore it is entirely
    superfluous and we can always pass napi->weight down to
    the ->poll() routine, and kill off napi->quota and all the
    synchronization problems.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index bc88e4c..cf89ce6 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -294,7 +294,6 @@ struct napi_struct {
 
 	unsigned long		state;
 	int			weight;
-	int			quota;
 	int			(*poll)(struct napi_struct *, int);
 #ifdef CONFIG_NETPOLL
 	spinlock_t		poll_lock;
diff --git a/net/core/dev.c b/net/core/dev.c
index 471e59d..0b9f82e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2107,13 +2107,6 @@ static int process_backlog(struct napi_struct *napi, int quota)
 	return work;
 }
 
-static void napi_check_quota_bug(struct napi_struct *n)
-{
-	/* It is illegal to consume more than the given quota.  */
-	if (WARN_ON_ONCE(n->quota < 0))
-		n->quota = 0;
-}
-
 /**
  * __napi_schedule - schedule for receive
  * @napi: entry to schedule
@@ -2124,9 +2117,6 @@ void fastcall __napi_schedule(struct napi_struct *n)
 {
 	unsigned long flags;
 
-	napi_check_quota_bug(n);
-	n->quota = n->weight;
-
 	local_irq_save(flags);
 	list_add_tail(&n->poll_list, &__get_cpu_var(softnet_data).poll_list);
 	__raise_softirq_irqoff(NET_RX_SOFTIRQ);
@@ -2146,6 +2136,7 @@ static void net_rx_action(struct softirq_action *h)
 
 	while (!list_empty(list)) {
 		struct napi_struct *n;
+		int work;
 
 		/* If softirq window is exhuasted then punt.
 		 *
@@ -2168,27 +2159,21 @@ static void net_rx_action(struct softirq_action *h)
 
 		have = netpoll_poll_lock(n);
 
-		/* if quota not exhausted process work */
-		if (likely(n->quota > 0)) {
-			int work = n->poll(n, min(budget, n->quota));
+		work = n->poll(n, n->weight);
 
-			budget -= work;
-			n->quota -= work;
-		}
+		WARN_ON_ONCE(work > n->weight);
 
-		local_irq_disable();
+		budget -= work;
 
-		napi_check_quota_bug(n);
+		local_irq_disable();
 
 		/* Drivers must not modify the NAPI state if they
-		 * consume the entire quota.  In such cases this code
+		 * consume the entire weight.  In such cases this code
 		 * still "owns" the NAPI instance and therefore can
 		 * move the instance around on the list at-will.
 		 */
-		if (unlikely(!n->quota)) {
-			n->quota = n->weight;
+		if (unlikely(work == n->weight))
 			list_move_tail(&n->poll_list, list);
-		}
 
 		netpoll_poll_unlock(have);
 	}

^ permalink raw reply related

* Re: bnx2 dirver's firmware images
From: Sam Ravnborg @ 2007-09-19 17:37 UTC (permalink / raw)
  To: maximilian attems
  Cc: H. Peter Anvin, David Miller, mchan, vda.linux, linux-kernel,
	netdev
In-Reply-To: <20070919171841.GT19918@baikonur.stro.at>

Hi Maks.

On Wed, Sep 19, 2007 at 07:18:41PM +0200, maximilian attems wrote:
> On Wed, Sep 19, 2007 at 10:12:38AM -0700, H. Peter Anvin wrote:
> > 
> > Could you give a concrete example?
> > 
> > 	-hpa
> 
> max@dual:~/src/klibc$ touch usr/utils/mount_main.c
> max@dual:~/src/klibc$ make
>   KLIBCCC usr/utils/mount_main.o
>   KLIBCLD usr/utils/static/halt
>   LN      usr/utils/static/reboot
>   LN      usr/utils/static/poweroff
>   KLIBCLD usr/utils/shared/halt
>   LN      usr/utils/shared/reboot
>   LN      usr/utils/shared/poweroff
>   KLIBCLD usr/utils/static/chroot
>   KLIBCLD usr/utils/static/dd
>   <snipp>

I do not recall the details at the moment but I recall I had to
do this tradeoff for some reasons.
I do not say it is not fixable but when I did the static-y support
I recall I took a shortcut or two under the assumption that everything
build by klibc was anyway so simple that compiling a bit too much was no harm.

Too buried with other stuff right now.
But feel free to poke me in roughly a month and I will take a deeper look.

	Sam

^ permalink raw reply

* Re: [PATCH] tehuti: driver for Tehuti 10GbE network adapters
From: 'Christoph Hellwig' @ 2007-09-19 17:41 UTC (permalink / raw)
  To: Alexander Indenbaum
  Cc: 'Christoph Hellwig', jeff, davem, akpm, netdev,
	'Nadav Shemer', 'Andy Gospodarek'
In-Reply-To: <20070919173724.C810375CC7@mail.tehutinetworks.co.il>

On Wed, Sep 19, 2007 at 07:37:18PM +0200, Alexander Indenbaum wrote:
> In my understanding unregister_netdev(), in case netdev is still IFF_UP and
> holds irq, will call dev_close() which will call netdev->stop(), bdx_close()
> in our case. bdx_close() releases all netdev resources among others calls
> free_irq().
> 
> That's why I'm pretty sure that we do not hold any reference to netdev after
> unregister_netdev() finishes and we can free_netdev() without any worry :)
> 
> What do you think, does it still look fishy?

Ok, so you do irq acquire/release in open/close so that bit is fine.
The code might actually work as is, sorry for the noise.

^ 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