* [PATCH 3/3] drivers/net/ipg.c: fix horrible mdio_read and _write
From: linux @ 2008-01-08 12:40 UTC (permalink / raw)
To: netdev, romieu; +Cc: akpm, davem, linux
akpm noticed that this code was awful.
ipg.c | 158 +++++++++++++++++------------------------------------------------- 1 file changed, 43 insertions(+), 115 deletions(-)
should be sufficient justification all by itself.
---
diff --git a/drivers/net/ipg.c b/drivers/net/ipg.c
index 3860fcd..b3d3fc8 100644
--- a/drivers/net/ipg.c
+++ b/drivers/net/ipg.c
@@ -202,12 +202,31 @@ static u16 read_phy_bit(void __iomem * ioaddr, u8 phyctrlpolarity)
}
/*
+ * Transmit the given bits, MSB-first, through the MgmtData bit (bit 1)
+ * of the PhyCtrl register. 1 <= len <= 32. "ioaddr" is the register
+ * address, and "otherbits" are the values of the other bits.
+ */
+static void mdio_write_bits(void __iomem *ioaddr, u8 otherbits, u32 data, unsigned len)
+{
+ otherbits |= IPG_PC_MGMTDIR;
+ do {
+ /* IPG_PC_MGMTDATA is a power of 2; compiler knows to shift */
+ u8 d = ((data >> --len) & 1) * IPG_PC_MGMTDATA;
+ /* + rather than | lets compiler microoptimize better */
+ ipg_drive_phy_ctl_low_high(ioaddr, d + otherbits);
+ } while (len);
+}
+
+/*
* Read a register from the Physical Layer device located
* on the IPG NIC, using the IPG PHYCTRL register.
*/
static int mdio_read(struct net_device * dev, int phy_id, int phy_reg)
{
void __iomem *ioaddr = ipg_ioaddr(dev);
+ u8 const polarity = ipg_r8(PHY_CTRL) &
+ (IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY);
+ unsigned i, data = 0;
/*
* The GMII mangement frame structure for a read is as follows:
*
@@ -221,75 +240,30 @@ static int mdio_read(struct net_device * dev, int phy_id, int phy_reg)
* D = bit of read data (MSB first)
*
* Transmission order is 'Preamble' field first, bits transmitted
- * left to right (first to last).
+ * left to right (msbit-first).
*/
- struct {
- u32 field;
- unsigned int len;
- } p[] = {
- { GMII_PREAMBLE, 32 }, /* Preamble */
- { GMII_ST, 2 }, /* ST */
- { GMII_READ, 2 }, /* OP */
- { phy_id, 5 }, /* PHYAD */
- { phy_reg, 5 }, /* REGAD */
- { 0x0000, 2 }, /* TA */
- { 0x0000, 16 }, /* DATA */
- { 0x0000, 1 } /* IDLE */
- };
- unsigned int i, j;
- u8 polarity, data;
-
- polarity = ipg_r8(PHY_CTRL);
- polarity &= (IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY);
-
- /* Create the Preamble, ST, OP, PHYAD, and REGAD field. */
- for (j = 0; j < 5; j++) {
- for (i = 0; i < p[j].len; i++) {
- /* For each variable length field, the MSB must be
- * transmitted first. Rotate through the field bits,
- * starting with the MSB, and move each bit into the
- * the 1st (2^1) bit position (this is the bit position
- * corresponding to the MgmtData bit of the PhyCtrl
- * register for the IPG).
- *
- * Example: ST = 01;
- *
- * First write a '0' to bit 1 of the PhyCtrl
- * register, then write a '1' to bit 1 of the
- * PhyCtrl register.
- *
- * To do this, right shift the MSB of ST by the value:
- * [field length - 1 - #ST bits already written]
- * then left shift this result by 1.
- */
- data = (p[j].field >> (p[j].len - 1 - i)) << 1;
- data &= IPG_PC_MGMTDATA;
- data |= polarity | IPG_PC_MGMTDIR;
-
- ipg_drive_phy_ctl_low_high(ioaddr, data);
- }
- }
-
- send_three_state(ioaddr, polarity);
-
- read_phy_bit(ioaddr, polarity);
+ mdio_write_bits(ioaddr, polarity, GMII_PREAMBLE, 32);
+ mdio_write_bits(ioaddr, polarity, GMII_ST<<12 | GMII_READ << 10 |
+ phy_id << 5 | phy_reg, 14);
/*
* For a read cycle, the bits for the next two fields (TA and
* DATA) are driven by the PHY (the IPG reads these bits).
*/
- for (i = 0; i < p[6].len; i++) {
- p[6].field |=
- (read_phy_bit(ioaddr, polarity) << (p[6].len - 1 - i));
- }
+ send_three_state(ioaddr, polarity); /* TA first bit */
+ (void)read_phy_bit(ioaddr, polarity); /* TA second bit */
+
+ for (i = 0; i < 16; i++)
+ data += data + read_phy_bit(ioaddr, polarity);
+ /* Trailing idle */
send_three_state(ioaddr, polarity);
send_three_state(ioaddr, polarity);
send_three_state(ioaddr, polarity);
send_end(ioaddr, polarity);
/* Return the value of the DATA field. */
- return p[6].field;
+ return data;
}
/*
@@ -299,11 +273,13 @@ static int mdio_read(struct net_device * dev, int phy_id, int phy_reg)
static void mdio_write(struct net_device *dev, int phy_id, int phy_reg, int val)
{
void __iomem *ioaddr = ipg_ioaddr(dev);
+ u8 const polarity = ipg_r8(PHY_CTRL) &
+ (IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY);
/*
- * The GMII mangement frame structure for a read is as follows:
+ * The GMII mangement frame structure for a write is as follows:
*
* |Preamble|st|op|phyad|regad|ta| data |idle|
- * |< 32 1s>|01|10|AAAAA|RRRRR|z0|DDDDDDDDDDDDDDDD|z |
+ * |< 32 1s>|01|01|AAAAA|RRRRR|10|DDDDDDDDDDDDDDDD|z |
*
* <32 1s> = 32 consecutive logic 1 values
* A = bit of Physical Layer device address (MSB first)
@@ -312,66 +288,18 @@ static void mdio_write(struct net_device *dev, int phy_id, int phy_reg, int val)
* D = bit of write data (MSB first)
*
* Transmission order is 'Preamble' field first, bits transmitted
- * left to right (first to last).
+ * left to right (msbit-first).
*/
- struct {
- u32 field;
- unsigned int len;
- } p[] = {
- { GMII_PREAMBLE, 32 }, /* Preamble */
- { GMII_ST, 2 }, /* ST */
- { GMII_WRITE, 2 }, /* OP */
- { phy_id, 5 }, /* PHYAD */
- { phy_reg, 5 }, /* REGAD */
- { 0x0002, 2 }, /* TA */
- { val & 0xffff, 16 }, /* DATA */
- { 0x0000, 1 } /* IDLE */
- };
- unsigned int i, j;
- u8 polarity, data;
-
- polarity = ipg_r8(PHY_CTRL);
- polarity &= (IPG_PC_DUPLEX_POLARITY | IPG_PC_LINK_POLARITY);
-
- /* Create the Preamble, ST, OP, PHYAD, and REGAD field. */
- for (j = 0; j < 7; j++) {
- for (i = 0; i < p[j].len; i++) {
- /* For each variable length field, the MSB must be
- * transmitted first. Rotate through the field bits,
- * starting with the MSB, and move each bit into the
- * the 1st (2^1) bit position (this is the bit position
- * corresponding to the MgmtData bit of the PhyCtrl
- * register for the IPG).
- *
- * Example: ST = 01;
- *
- * First write a '0' to bit 1 of the PhyCtrl
- * register, then write a '1' to bit 1 of the
- * PhyCtrl register.
- *
- * To do this, right shift the MSB of ST by the value:
- * [field length - 1 - #ST bits already written]
- * then left shift this result by 1.
- */
- data = (p[j].field >> (p[j].len - 1 - i)) << 1;
- data &= IPG_PC_MGMTDATA;
- data |= polarity | IPG_PC_MGMTDIR;
-
- ipg_drive_phy_ctl_low_high(ioaddr, data);
- }
- }
+ mdio_write_bits(ioaddr, polarity, GMII_PREAMBLE, 32);
+ mdio_write_bits(ioaddr, polarity, GMII_ST << 14 | GMII_WRITE << 12 |
+ phy_id << 7 | phy_reg << 2 |
+ 0x2, 16);
+ mdio_write_bits(ioaddr, polarity, val, 16); /* DATA */
/* The last cycle is a tri-state, so read from the PHY. */
- for (j = 7; j < 8; j++) {
- for (i = 0; i < p[j].len; i++) {
- ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_LO | polarity);
-
- p[j].field |= ((ipg_r8(PHY_CTRL) &
- IPG_PC_MGMTDATA) >> 1) << (p[j].len - 1 - i);
-
- ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_HI | polarity);
- }
- }
+ ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_LO | polarity);
+ (void)ipg_r8(PHY_CTRL);
+ ipg_write_phy_ctl(ioaddr, IPG_PC_MGMTCLK_HI | polarity);
}
/* Set LED_Mode JES20040127EEPROM */
^ permalink raw reply related
* Re: [RFC PATCH v2 2/2] SELinux: Add network ingress and egress control permission checks
From: Paul Moore @ 2008-01-08 13:13 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20080107.220211.250933730.davem@davemloft.net>
On Tuesday 08 January 2008 1:02:11 am David Miller wrote:
> From: Paul Moore <paul.moore@hp.com>
> Date: Mon, 07 Jan 2008 12:47:48 -0500
>
> > This patch implements packet ingress/egress controls for SELinux which
> > allow SELinux security policy to control the flow of all IPv4 and IPv6
> > packets into and out of the system. Currently SELinux does not have
> > proper control over forwarded packets and this patch corrects this
> > problem.
> >
> > Special thanks to Venkat Yekkirala <vyekkirala@trustedcs.com> whose
> > earlier work on this topic eventually led to this patch.
> >
> > Signed-off-by: Paul Moore <paul.moore@hp.com>
>
> This looks fine, and since it doesn't touch anything under net/
> please feel free to merge it however you like.
Thanks. For the record, I believe the plan is that James will be pushing all
the labeled networking changes to Linus when the time comes.
--
paul moore
linux security @ hp
^ permalink raw reply
* Re: [IPV4] ROUTE: Avoid sparse warnings
From: Jarek Poplawski @ 2008-01-08 12:59 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <47832BE6.5070906@cosmosbay.com>
On 08-01-2008 08:53, Eric Dumazet wrote:
> David Miller a écrit :
...
>> Furthermore, these:
>>
>> rcu_read_unlock_bh()
>> rcu_read_lock_bh()
>>
>> sequences are at best funny looking. For other lock types we would
>> look at this and ask "Does this even accomplish anything reliably?"
>
> Well, original code exactly does the same thing.
>
>>
>> The answer here is that it wants the preempt_enable() to run to get
>> any potential kernel preemptions executed. It also allows any
>> pending software interrupts to run.
>>
>> So this does something reliably only because rcu_read_unlock_bh() has
>> specific and explicit side effects.
>>
>
> I will post a patch to introduce a helper function, so that this is
> clearly documented and not relying on side effects. Actual
> implementation has latency
> problems on empty hash tables if CONFIG_PREEMPT=n
>
>
>> I don't know, to me it just looks awful :-) I better understood the
>> original code.
It seems this patch only made it more visible how it currently works.
I don't know what changes do you plan for this helper function, but
my proposal is to add some counter and break this rcu only after
looping for some time. Alternatively cpu_relax() could be probably
used between these "locks". Without this probably some cache problems
are possible, but you know this better, I guess.
Regards,
Jarek P.
^ permalink raw reply
* packet corruption or something else?
From: Denys Fedoryshchenko @ 2008-01-08 12:49 UTC (permalink / raw)
To: netdev
Hi
I notice a lot of messages like
[8447790.549705] UDP: short packet: From XXX.XXX.224.29:21005 60046/1480 to
XXX.XXX.247.1:1024
[8448040.893317] UDP: short packet: From XXX.XXX.224.29:21218 17820/1480 to
XXX.XXX.247.1:49974
[8448216.603759] UDP: short packet: From XXX.XXX.224.29:21004 56347/1480 to
XXX.XXX.247.1:1024
[8448370.883610] UDP: short packet: From XXX.XXX.224.29:21005 5246/1480 to
XXX.XXX.247.1:1251
I have similar messages on other hosts, and they looks very strange.
After looking to kernel sources i found
ulen = ntohs(uh->len);
if (ulen > skb->len)
goto short_packet;
if (proto == IPPROTO_UDP) {
/* UDP validates ulen. */
if (ulen < sizeof(*uh) || pskb_trim_rcsum(skb, ulen))
goto short_packet;
uh = udp_hdr(skb);
}
Means if specified length in packet header more large than len in skb, go to
short_packet.
As i know for sure, i have running UDP application which sending packets not
more than 1500 bytes (1480 actually without header), and it is running on
ports 21005, 21004, 21006. There is for sure impossible it will send data
from port 21218 and impossible it will send packet larger than 1480 bytes.
Receiving side is Intel card with hardware checksumming enabled. So in theory
corrupted packets must not bypass hardware checksumming (but probably i am
wrong). Is there anything suspicious and need additional attention?
--
Denys Fedoryshchenko
Technical Manager
Virtual ISP S.A.L.
^ permalink raw reply
* Re: SACK scoreboard
From: Ilpo Järvinen @ 2008-01-08 12:12 UTC (permalink / raw)
To: David Miller; +Cc: lachlan.andrew, Netdev, quetchen
In-Reply-To: <20080107.233617.203640686.davem@davemloft.net>
On Mon, 7 Jan 2008, David Miller wrote:
> Did you happen to read a recent blog posting of mine?
>
> http://vger.kernel.org/~davem/cgi-bin/blog.cgi/2007/12/31#tcp_overhead
>
> I've been thinking more and more and I think we might be able
> to get away with enforcing that SACKs are always increasing in
> coverage.
>
> I doubt there are any real systems out there that drop out of order
> packets that are properly formed and are in window, even though the
> SACK specification (foolishly, in my opinion) allows this.
Luckily we can see that already from MIBs, so quering people who have
large servers, which are continously "testing" the internet :-), under
their supervision or can access, and asking if they see any might help.
I checked my dept's interactive servers and all had zero renegings, but
I don't think I have access to www server which would have much wider
exposure.
> If we could free packets as SACK blocks cover them, all the problems
> go away.
I thought it a bit yesterday after reading your blog and came to
conclusion that they won't, we can still get those nasty ACKs regardless
of received SACK info (in here, missing). Even in some valid cases which
include ACK losses besides actual data loss, not that this is the most
common case but just wanted to point out that cleanup work is at least
partially independent of SACK problem. So not "all" problems would go
away really.
> For one thing, this will allow the retransmit queue liberation during
> loss recovery to be spread out over the event, instead of batched up
> like crazy to the point where the cumulative ACK finally moves and
> releases an entire window's worth of data.
Two key cases for real pattern are:
1. Losses once per n, where n is something small, like 2-20 or so, usually
happens at slow start overshoot or when compething traffic slow starts.
Cumulative ACKs will cover only small part of the window once rexmits
make through, thus this is not a problem.
2. Single loss (or few at the beginning of the window), rest SACKed.
Cumulative ACK will cover original window when the last necessary
rexmit gets through.
Case 1 becomes nasty ACKy only if rexmit is lost as well, but in that case
the arriving SACK blocks make the rest of the window equal to 2 :-).
So I'm now trying to solve just case 2. What if we could somehow "combine"
adjacent skbs (or whatever they're called in that model) if SACK covers
them both so that we still hold them but can drop them in a very
efficient way. That would make the combining effort split per ACK.
And if reneging would occur, we can think a way to put the necessary fuzz
into a form which cannot hurt the rest of the system (relatively easy &
fast if we add CA_Reneging and allow retransmitting a portion of an skb
similar to what you suggested earlier).
And it might even be possible then to offer admin a control so that the
admin can choose between recover/plain reset if admin thinks that it's
always an indication of an attack. This is somewhat similar case to what
UTO (under IETF evaluation) does, as purpose of both is in violation of
RFC TCP to avoid malicious traps but the control about it is left to the
user.
> Next, it would simplify all of this scanning code trying to figure out
> which holes to fill during recovery.
>
> And for SACK scoreboard marking, the RB trie would become very nearly
> unecessary as far as I can tell.
I've been contacted by a person who was interested in reaching 500k
windows, so your 4000 sounded like a joke :-/. Having, let say, every
20th dropped means 25k skbs remaining, can we scan though it in any
sensible time without RBs and friends :-)? However, allowing queue walk
to begin from either direction would solve most of the common cases well
enough for it to be nearly manageable.
> I would not even entertain this kind of crazy idea unless I thought
> the fundamental complexity simplification payback was enormous. And
> in this case I think it is.
>
> What we could do is put some experimental hack in there for developers
> to start playing with, which would enforce that SACKs always increase
> in coverage. If violated the connection reset and a verbose log
> message is logged so we can analyze any cases that occur.
We have an initial number already, in MIBs.
> Sounds crazy, but maybe has potential. What do you think?
If I'd hint my boss that I'm involved in something like this I'd
bet that he also would get quite crazy... ;-) I'm partially paid
for making TCP more RFCish :-), or at least that the places where
thing diverge are known and controllable for research purposes.
--
i.
ps. If other Cced would like to get dropped if there are some followups,
just let me know :-). Else, no need to do anything.
^ permalink raw reply
* [VLAN]: Avoid expensive divides
From: Eric Dumazet @ 2008-01-08 11:28 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev
[-- Attachment #1: Type: text/plain, Size: 222 bytes --]
We can avoid divides (as seen with CONFIG_CC_OPTIMIZE_FOR_SIZE=y on x86)
changing vlan_group_get_device()/vlan_group_set_device() id parameter
from signed to
unsigned.
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
[-- Attachment #2: if_vlan.patch --]
[-- Type: text/plain, Size: 827 bytes --]
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 976d4b1..4562105 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -81,14 +81,16 @@ struct vlan_group {
struct rcu_head rcu;
};
-static inline struct net_device *vlan_group_get_device(struct vlan_group *vg, int vlan_id)
+static inline struct net_device *vlan_group_get_device(struct vlan_group *vg,
+ unsigned int vlan_id)
{
struct net_device **array;
array = vg->vlan_devices_arrays[vlan_id / VLAN_GROUP_ARRAY_PART_LEN];
return array[vlan_id % VLAN_GROUP_ARRAY_PART_LEN];
}
-static inline void vlan_group_set_device(struct vlan_group *vg, int vlan_id,
+static inline void vlan_group_set_device(struct vlan_group *vg,
+ unsigned int vlan_id,
struct net_device *dev)
{
struct net_device **array;
^ permalink raw reply related
* Re: [PATCH 6/8] [PATCH] Split up rndis_host.c
From: David Brownell @ 2008-01-08 11:19 UTC (permalink / raw)
To: Jussi Kivilinna; +Cc: bjd, netdev, linux-wireless
In-Reply-To: <1199304827.8532.31.camel@localhost>
On Wednesday 02 January 2008, Jussi Kivilinna wrote:
> Hello,
>
> Bjorge was not comfortable with double probing rndis_wext requires,
> wireless RNDIS devices have same device id as the rest of RNDIS.
I should have known Microsoft wouldn't start by assuming systems
software module boundaries should be natural and obvious ones. :(
I see that the rndis_wext.c Kconfig won't kick in unless the
802.11 stuff is available ... what additional dependencies
does that imply for a fatter rndis_host module? If it doesn't
cause extra modules to be loaded (even for non-WLAN devices),
then I suppose I could just live with the RNDIS module being
bigger than it needs to be. It was pretty thin to start with!
What would bother me would be seeing that systems with this
WLAN support option configured could no longer load the basic
RNDIS code without also loading a bunch of extra modules
that weren't needed for all RNDIS devices.
> Our
> module version checks OID_GEN_PHYSICAL_MEDIUM in generic_rndis_bind,
> with rndis_host bind fails if OID is supported and wireless media type
> is returned, with rndis_wext if OID isn't supported or type isn't
> wireless. Should this be ok?
Sounds like if you can go the different-modules route, then rndis_bind()
should maybe accept a parameter giving it that option. Then
rndis_wlan_bind(...) == rndis_bind(..., check_wlan_media)
rndis_generic_bind(...) == rndis_bind(..., non_wlan_media)
That would be getting pretty complicated, since as you say the same
USB-level binding (hence hotplugging) would need to kick in ... but
having different modules for different RNDIS flavors would need a
new hotplug mechanism keying on media type. Ugh.
> Should separate rndis_wext be located in drivers/net/wireless instead of
> drivers/net/usb?
If that doesn't complicate maintaining that software, I can't
see why it shouldn't go there. It might imply moving the usbnet
interface declarations to someplace linux/include though. (And
that presumes there's actually a sane way to split that stuff
into its own module, too...)
- Dave
> - Jussi Kivilinna
>
> On Sat, 2007-12-22 at 17:17 -0800, David Brownell wrote:
> > > From: Bjorge Dijkstra <bjd@jooz.net>
> > > Subject: [PATCH 6/8] [PATCH] Split up rndis_host.c
> > > Date: Sat, 22 Dec 2007 22:51:32 +0100
> > >
> > > Split up rndis_host.c into rndis_host.h and rndis_base.c and
> > > change Makefile accordingly. This is done so we can add extra
> > > source files to the rndis_host module later on.
> >
> > I'm fine with splitting out a header file and the EXPORT_SYMBOL_GPL.
> > But why not just have a separate "rndis_wext" module?
> >
> >
> > > ---
> > > drivers/net/usb/Makefile | 1 +
> > > drivers/net/usb/rndis_base.c | 548 ++++++++++++++++++++++++++++++
> > > drivers/net/usb/rndis_host.c | 763 ------------------------------------------
> > > drivers/net/usb/rndis_host.h | 256 ++++++++++++++
> > > 4 files changed, 805 insertions(+), 763 deletions(-)
> > > create mode 100644 drivers/net/usb/rndis_base.c
> > > delete mode 100644 drivers/net/usb/rndis_host.c
> > > create mode 100644 drivers/net/usb/rndis_host.h
> >
>
^ permalink raw reply
* Re: [PATCH 3/4] [XFRM]: Kill some bloat
From: Ilpo Järvinen @ 2008-01-08 10:57 UTC (permalink / raw)
To: Andi Kleen
Cc: David Miller, Herbert Xu, Netdev, Arnaldo Carvalho de Melo,
paul.moore, latten
In-Reply-To: <p73sl19qi6o.fsf@bingen.suse.de>
On Tue, 8 Jan 2008, Andi Kleen wrote:
> David Miller <davem@davemloft.net> writes:
>
> > Similarly I question just about any inline usage at all in *.c files
>
> Don't forget the .h files. Especially a lot of stuff in tcp.h should
> be probably in some .c file and not be inline.
I'm not forgetting them... :-)
My intention is to do the same for all headers as well but I'd really like
to get some actual number of bytes by measuring rather than taking them of
the hat.
...It just requires some work still to get the uninlining actually do the
right thing and actually testing it involves orders of magnitude more
exhaustive recompilation than .o only checking required so after finishing
the script I either has to setup n distcc master with a number of slaves
each or wait for some time for the results :-).
However, I suspect that anything in tcp.h won't match to something like
~5-10k like I've seen in three .c files already + one with debugging that
has even more than 10k (even if all in tcp.h summed up :-)).
--
i.
^ permalink raw reply
* Re: [PATCH 3/4] [XFRM]: Kill some bloat
From: Ilpo Järvinen @ 2008-01-08 10:48 UTC (permalink / raw)
To: David Miller
Cc: andi, herbert, ilpo.jarvinen, netdev, acme, paul.moore, latten
In-Reply-To: <20080107.211018.160153013.davem@davemloft.net>
On Mon, 7 Jan 2008, David Miller wrote:
> From: Andi Kleen <andi@firstfloor.org>
> Date: Tue, 8 Jan 2008 06:00:07 +0100
>
> > On Mon, Jan 07, 2008 at 07:37:00PM -0800, David Miller wrote:
> > > The vast majority of them are one, two, and three liners.
> >
> > % awk ' { line++ } ; /^{/ { total++; start = line } ; /^}/ { len=line-start-3; if (len > 4) l++; if (len >= 10) k++; } ; END { print total, l, l/total, k, k/total }' < include/net/tcp.h
> > 68 28 0.411765 20 0.294118
> >
> > 41% are over 4 lines, 29% are >= 10 lines.
>
> Take out the comments and whitespace lines, your script is
> too simplistic.
He should also remove these, which involve just syntactic casting to
please the compiler:
struct tcp_sock *tp = tcp_sk(sk);
struct inet_connection_sock *icsk = inet_csk(sk);
...and maybe some other similar ones. I'm sure that's going to make his
statistics even more questionable as is because we need tp all around.
But about his concerns, I spend couple of minutes in looking to it by
hand. These are likely to be a win:
tcp_set_state
tcp_prequeue
tcp_prequeue_init
tcp_openreq_init
...about rest I'm very unsure but there are some that probably are a
minor win as well.
tcp_dec_quickack_mode has only a single caller anyway so I'd doubt that
it's going to be significant, I thought moving it to .c earlier but
haven't just done that yet :-).
--
i.
^ permalink raw reply
* Re: [XFRM]: xfrm_state_clone() should be static, not exported
From: David Miller @ 2008-01-08 9:36 UTC (permalink / raw)
To: dada1; +Cc: netdev
In-Reply-To: <47832D94.3000302@cosmosbay.com>
From: Eric Dumazet <dada1@cosmosbay.com>
Date: Tue, 08 Jan 2008 09:00:20 +0100
> xfrm_state_clone() is not used outside of net/xfrm/xfrm_state.c
> There is no need to export it.
>
> Spoted by sparse checker.
> CHECK net/xfrm/xfrm_state.c
> net/xfrm/xfrm_state.c:1103:19: warning: symbol 'xfrm_state_clone' was not
> declared. Should it be static?
>
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
Applied to net-2.6.25, thanks Eric.
^ permalink raw reply
* Re: [IPV4] ROUTE: Avoid sparse warnings
From: David Miller @ 2008-01-08 9:34 UTC (permalink / raw)
To: dada1; +Cc: netdev
In-Reply-To: <47832BE6.5070906@cosmosbay.com>
From: Eric Dumazet <dada1@cosmosbay.com>
Date: Tue, 08 Jan 2008 08:53:10 +0100
> It warns us because rt_cache_get_first() can returns with RCU_BH
> *acquired* or not.
As Herbert mentioned that's a pretty often used paradigm called
conditional locking. :-)
^ permalink raw reply
* Re: [PATCH] AX25: nuke user trigable printks
From: maximilian attems @ 2008-01-08 9:21 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20080107.215550.08459948.davem@davemloft.net>
On Mon, Jan 07, 2008 at 09:55:50PM -0800, David Miller wrote:
>
> Can you replace the printk()'s with comments at least?
>
> Otherwise nobody is going to remember what we were trying
> to accomplish here and it will guarentee that, in fact,
> support for these old things will never get removed.
>
> At least if comments are there, someone might think to add
> these things to the feature removal schedule and follow
> through with it.
sure will resend later.
wasn't sure myself of the chosen minimal user visible clean up.
thanks
--
maks
^ permalink raw reply
* Re: 2.6.23-rc8 network problem. Mem leak? ip1000a?
From: Francois Romieu @ 2008-01-08 7:51 UTC (permalink / raw)
To: David Miller; +Cc: linux, akpm, netdev
In-Reply-To: <20080107.231447.08811264.davem@davemloft.net>
David Miller <davem@davemloft.net> :
[...]
> I invested 30 seconds of code reading to figure out the leak. A much
> better investment of time than adding bogus comments to the Kconfig
> help text don't you think? :-)
Thanks for the hint David.
I'll roll up a patch for it after the day work.
--
Ueimor
^ permalink raw reply
* [XFRM]: xfrm_state_clone() should be static, not exported
From: Eric Dumazet @ 2008-01-08 8:00 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
[-- Attachment #1: Type: text/plain, Size: 313 bytes --]
xfrm_state_clone() is not used outside of net/xfrm/xfrm_state.c
There is no need to export it.
Spoted by sparse checker.
CHECK net/xfrm/xfrm_state.c
net/xfrm/xfrm_state.c:1103:19: warning: symbol 'xfrm_state_clone' was not
declared. Should it be static?
Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
[-- Attachment #2: xfrm_state.patch --]
[-- Type: text/plain, Size: 716 bytes --]
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 81b0fce..5fc8217 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1100,7 +1100,7 @@ out:
EXPORT_SYMBOL(xfrm_state_add);
#ifdef CONFIG_XFRM_MIGRATE
-struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, int *errp)
+static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, int *errp)
{
int err = -ENOMEM;
struct xfrm_state *x = xfrm_state_alloc();
@@ -1175,7 +1175,6 @@ struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, int *errp)
kfree(x);
return NULL;
}
-EXPORT_SYMBOL(xfrm_state_clone);
/* xfrm_state_lock is held */
struct xfrm_state * xfrm_migrate_state_find(struct xfrm_migrate *m)
^ permalink raw reply related
* Re: [IPV4] ROUTE: Avoid sparse warnings
From: Eric Dumazet @ 2008-01-08 7:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20080107.225417.119463845.davem@davemloft.net>
David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Mon, 7 Jan 2008 12:01:17 +0100
>
>> CHECK net/ipv4/route.c
>> net/ipv4/route.c:298:2: warning: context imbalance in 'rt_cache_get_first' - wrong count at exit
>> net/ipv4/route.c:307:3: warning: context imbalance in 'rt_cache_get_next' - unexpected unlock
>> net/ipv4/route.c:346:3: warning: context imbalance in 'rt_cache_seq_stop' - unexpected unlock
>>
>> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>>
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index 10915bb..fec0571 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -289,11 +289,11 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
>> struct rt_cache_iter_state *st = seq->private;
>>
>> for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
>> - rcu_read_lock_bh();
>> r = rt_hash_table[st->bucket].chain;
>> if (r)
>> break;
>> rcu_read_unlock_bh();
>> + rcu_read_lock_bh();
>> }
>> return r;
>> }
>
> Like for Herbert, this patch leaves a bad taste in my mouth.
>
> I don't understand, is it the case that sparse doesn't like
> that rt_cache_get_first() returns with rcu_read_lock_bh()
> held? That's kind of rediculious.
Not exactly.
It warns us because rt_cache_get_first() can returns with RCU_BH *acquired* or
not. I find this warning very usefull. In rt_cache_get_first() this warning
is a false alarm.
>
> Furthermore, these:
>
> rcu_read_unlock_bh()
> rcu_read_lock_bh()
>
> sequences are at best funny looking. For other lock types we would
> look at this and ask "Does this even accomplish anything reliably?"
Well, original code exactly does the same thing.
>
> The answer here is that it wants the preempt_enable() to run to get
> any potential kernel preemptions executed. It also allows any
> pending software interrupts to run.
>
> So this does something reliably only because rcu_read_unlock_bh() has
> specific and explicit side effects.
>
I will post a patch to introduce a helper function, so that this is clearly
documented and not relying on side effects. Actual implementation has latency
problems on empty hash tables if CONFIG_PREEMPT=n
> I don't know, to me it just looks awful :-) I better understood the
> original code.
>
> We can continue splitting hairs over this but I'll hold off on this
> patch for now. :)
Fair enough
Thanks
^ permalink raw reply
* Re: SACK scoreboard
From: David Miller @ 2008-01-08 7:36 UTC (permalink / raw)
To: ilpo.jarvinen; +Cc: lachlan.andrew, netdev, quetchen
In-Reply-To: <Pine.LNX.4.64.0712111547300.18529@kivilampi-30.cs.helsinki.fi>
Ilpo, just trying to keep an old conversation from dying off.
Did you happen to read a recent blog posting of mine?
http://vger.kernel.org/~davem/cgi-bin/blog.cgi/2007/12/31#tcp_overhead
I've been thinking more and more and I think we might be able
to get away with enforcing that SACKs are always increasing in
coverage.
I doubt there are any real systems out there that drop out of order
packets that are properly formed and are in window, even though the
SACK specification (foolishly, in my opinion) allows this.
If we could free packets as SACK blocks cover them, all the problems
go away.
For one thing, this will allow the retransmit queue liberation during
loss recovery to be spread out over the event, instead of batched up
like crazy to the point where the cumulative ACK finally moves and
releases an entire window's worth of data.
Next, it would simplify all of this scanning code trying to figure out
which holes to fill during recovery.
And for SACK scoreboard marking, the RB trie would become very nearly
unecessary as far as I can tell.
I would not even entertain this kind of crazy idea unless I thought
the fundamental complexity simplification payback was enormous. And
in this case I think it is.
What we could do is put some experimental hack in there for developers
to start playing with, which would enforce that SACKs always increase
in coverage. If violated the connection reset and a verbose log
message is logged so we can analyze any cases that occur.
Sounds crazy, but maybe has potential. What do you think?
^ permalink raw reply
* Re: Top 10 kernel oopses for the week ending January 5th, 2008
From: Jarek Poplawski @ 2008-01-08 7:33 UTC (permalink / raw)
To: Arjan van de Ven
Cc: Al Viro, Linus Torvalds, Kevin Winchester, J. Bruce Fields,
Linux Kernel Mailing List, Andrew Morton, NetDev, gregkh, neilb
In-Reply-To: <20080108055917.GZ27894@ZenIV.linux.org.uk>
On 08-01-2008 06:59, Al Viro wrote:
> On Mon, Jan 07, 2008 at 07:26:12PM -0800, Linus Torvalds wrote:
>
>> I usually just compile a small program like
>>
>> const char array[]="\xnn\xnn\xnn...";
>>
>> int main(int argc, char **argv)
>> {
>> printf("%p\n", array);
>> *(int *)0=0;
>> }
> Heh. I prefer
> char main[] = {.....};
> for the same thing, with gdb a.out and no running at all.
...
IMHO, it would be really wasteful if Arian havn't published these
and maybe a few more such advices and links on this site, not
necessarily waiting for html-ized or howto-ized versions!
Thanks,
Jarek P.
^ permalink raw reply
* [NET] mcs7830 passes msecs instead of jiffies to usb_control_msg
From: Russ Dill @ 2008-01-08 7:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20080107.214826.153054474.davem@davemloft.net>
usb_control_msg was changed long ago (2.6.12-pre) to take milliseconds
instead of jiffies. Oddly, mcs7830 wasn't added until 2.6.19-rc3.
Signed-off-by: Russ Dill <Russ.Dill@asu.edu>
---
drivers/net/usb/mcs7830.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/mcs7830.c b/drivers/net/usb/mcs7830.c
index f55a595..5ea7411 100644
--- a/drivers/net/usb/mcs7830.c
+++ b/drivers/net/usb/mcs7830.c
@@ -94,7 +94,7 @@ static int mcs7830_get_reg(struct usbnet *dev, u16 index, u16 size, void *data)
ret = usb_control_msg(xdev, usb_rcvctrlpipe(xdev, 0), MCS7830_RD_BREQ,
MCS7830_RD_BMREQ, 0x0000, index, data,
- size, msecs_to_jiffies(MCS7830_CTRL_TIMEOUT));
+ size, MCS7830_CTRL_TIMEOUT);
return ret;
}
@@ -105,7 +105,7 @@ static int mcs7830_set_reg(struct usbnet *dev, u16 index, u16 size, void *data)
ret = usb_control_msg(xdev, usb_sndctrlpipe(xdev, 0), MCS7830_WR_BREQ,
MCS7830_WR_BMREQ, 0x0000, index, data,
- size, msecs_to_jiffies(MCS7830_CTRL_TIMEOUT));
+ size, MCS7830_CTRL_TIMEOUT);
return ret;
}
--
1.5.3.7
^ permalink raw reply related
* Re: forcedeth: MAC-address reversed on resume from suspend
From: David Miller @ 2008-01-08 7:23 UTC (permalink / raw)
To: B.Steinbrink
Cc: arbrandes, bunk, andi, richie, linux-kernel, aabdulla, jgarzik,
netdev
In-Reply-To: <20080107014638.GA4170@atjola.homenet>
From: Björn Steinbrink <B.Steinbrink@gmx.de>
Date: Mon, 7 Jan 2008 02:46:38 +0100
> On 2008.01.06 19:49:49 -0200, Adolfo R. Brandes wrote:
> > I have this forcedeth MAC address reversal problem when suspending
> > on 2 distinct boxes. I can confirm Steinbrink's patch fixes the
> > problem on only one of them:
> >
> > 00:0a.0 Bridge: nVidia Corporation CK804 Ethernet Controller (rev f3)
> >
> > On the other one the problem persists:
> >
> > 00:14.0 Bridge: nVidia Corporation MCP51 Ethernet Controller (rev a1)
>
> Thanks. Leaves me pretty clueless though. Especially since it worked for
> Richard, who also has a MCP51. In a private mail, you said that you had
> hardcoded the mac address in the source. Any chance that you applied the
> patch on your modified sources and didn't get it right?
I am going to push this patch upstream, it is correct and we
have a positive test case that failed before, so overall it's
a net improvement even if we still don't exactly know why
Adolfo's case still fails.
Let me know if there are any follow-on patches.
Thanks.
^ permalink raw reply
* Re: [NET] mcs7830 passes msecs instead of jiffies to usb_control_msg
From: David Miller @ 2008-01-08 7:16 UTC (permalink / raw)
To: Russ.Dill; +Cc: netdev
In-Reply-To: <1199776179.26870.1.camel@russ-laptop>
From: Russ Dill <Russ.Dill@asu.edu>
Date: Tue, 08 Jan 2008 00:09:39 -0700
> usb_control_msg was changed long ago (2.6.12-pre) to take milliseconds
> instead of jiffies. Oddly, mcs7830 wasn't added until 2.6.19-rc3.
>
> Signed-off-by: Russ Dill <Russ.Dill@asu.edu>
Applied, thanks Russ.
^ permalink raw reply
* Re: 2.6.23-rc8 network problem. Mem leak? ip1000a?
From: David Miller @ 2008-01-08 7:14 UTC (permalink / raw)
To: linux; +Cc: akpm, netdev, romieu
In-Reply-To: <20080107.230709.216880096.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Mon, 07 Jan 2008 23:07:09 -0800 (PST)
> From: linux@horizon.com
> Date: 8 Jan 2008 01:52:11 -0500
>
> > @@ -172,6 +172,10 @@ config IP1000
> > select MII
> > ---help---
> > This driver supports IP1000 gigabit Ethernet cards.
> > + It works, but suffers from a memory leak. Signifcant
> > + use will consume unswappable kernel memory until the
> > + machine runs out of memory and crashes. Thus, this
> > + driver cannot be considered usable at the the present time.
>
> This is not how we handle and track bugs.
>
> Such a patch is inappropriate, and I'd like to ask that you just be
> patient until someone has a chance to try and figure out what the
> problem is. Or even better, you can try to track down the problem
> yourself since you seem to have a specific interest in this problem.
Actually, the bug is amazingly obvious after a quick scan of this
driver.
ipg_nic_rx_free_skb() is called from various places and is given zero
context to work with. It assumes that the caller wants
"sp->rx_current % IPG_RFCLIST_LENGTH" to be freed.
But that's not right in most cases. For example, consider the call in
ipg_nic_rx_with_end(). This function is invoked from ipg_nic_rx()
like so:
unsigned int curr = sp->rx_current;
...
for (i = 0; i < IPG_MAXRFDPROCESS_COUNT; i++, curr++) {
unsigned int entry = curr % IPG_RFDLIST_LENGTH;
struct ipg_rx *rxfd = sp->rxd + entry;
if (!(rxfd->rfs & le64_to_cpu(IPG_RFS_RFDDONE)))
break;
switch (ipg_nic_rx_check_frame_type(dev)) {
...
case Frame_WithEnd:
ipg_nic_rx_with_end(dev, tp, rxfd, entry);
break;
...
}
}
sp->rx_current = curr;
So sp->rx_current does not correspond to the packet being processed
currently, so ipg_nic_rx_free_skb() will only look at and try to free
only the first packet the above loop tries to processe.
WOW!!!! Amazing!!!
I invested 30 seconds of code reading to figure out the leak. A much
better investment of time than adding bogus comments to the Kconfig
help text don't you think? :-)
^ permalink raw reply
* Re: 2.6.23-rc8 network problem. Mem leak? ip1000a?
From: David Miller @ 2008-01-08 7:07 UTC (permalink / raw)
To: linux; +Cc: akpm, netdev, romieu
In-Reply-To: <20080108065211.25290.qmail@science.horizon.com>
From: linux@horizon.com
Date: 8 Jan 2008 01:52:11 -0500
> @@ -172,6 +172,10 @@ config IP1000
> select MII
> ---help---
> This driver supports IP1000 gigabit Ethernet cards.
> + It works, but suffers from a memory leak. Signifcant
> + use will consume unswappable kernel memory until the
> + machine runs out of memory and crashes. Thus, this
> + driver cannot be considered usable at the the present time.
This is not how we handle and track bugs.
Such a patch is inappropriate, and I'd like to ask that you just be
patient until someone has a chance to try and figure out what the
problem is. Or even better, you can try to track down the problem
yourself since you seem to have a specific interest in this problem.
Thanks.
^ permalink raw reply
* Re: [IPV4] ROUTE: Avoid sparse warnings
From: David Miller @ 2008-01-08 6:54 UTC (permalink / raw)
To: dada1; +Cc: netdev
In-Reply-To: <20080107120117.aeebd0c8.dada1@cosmosbay.com>
From: Eric Dumazet <dada1@cosmosbay.com>
Date: Mon, 7 Jan 2008 12:01:17 +0100
> CHECK net/ipv4/route.c
> net/ipv4/route.c:298:2: warning: context imbalance in 'rt_cache_get_first' - wrong count at exit
> net/ipv4/route.c:307:3: warning: context imbalance in 'rt_cache_get_next' - unexpected unlock
> net/ipv4/route.c:346:3: warning: context imbalance in 'rt_cache_seq_stop' - unexpected unlock
>
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 10915bb..fec0571 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -289,11 +289,11 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
> struct rt_cache_iter_state *st = seq->private;
>
> for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
> - rcu_read_lock_bh();
> r = rt_hash_table[st->bucket].chain;
> if (r)
> break;
> rcu_read_unlock_bh();
> + rcu_read_lock_bh();
> }
> return r;
> }
Like for Herbert, this patch leaves a bad taste in my mouth.
I don't understand, is it the case that sparse doesn't like
that rt_cache_get_first() returns with rcu_read_lock_bh()
held? That's kind of rediculious.
Furthermore, these:
rcu_read_unlock_bh()
rcu_read_lock_bh()
sequences are at best funny looking. For other lock types we would
look at this and ask "Does this even accomplish anything reliably?"
The answer here is that it wants the preempt_enable() to run to get
any potential kernel preemptions executed. It also allows any
pending software interrupts to run.
So this does something reliably only because rcu_read_unlock_bh() has
specific and explicit side effects.
I don't know, to me it just looks awful :-) I better understood the
original code.
We can continue splitting hairs over this but I'll hold off on this
patch for now. :)
^ permalink raw reply
* Re: 2.6.23-rc8 network problem. Mem leak? ip1000a?
From: linux @ 2008-01-08 6:52 UTC (permalink / raw)
To: akpm, netdev, romieu; +Cc: linux
In-Reply-To: <20070930022347.37514be3.akpm@linux-foundation.org>
Just to keep the issue open, drivers/net/ipg.c currently in 2.6.24-rc6
still leaks skbuffs like a sieve. Run it for a few hours with network
traffic and the machine swaps like crazy while the oom killer goes nuts.
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index d9107e5..4fa392c 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -172,6 +172,10 @@ config IP1000
select MII
---help---
This driver supports IP1000 gigabit Ethernet cards.
+ It works, but suffers from a memory leak. Signifcant
+ use will consume unswappable kernel memory until the
+ machine runs out of memory and crashes. Thus, this
+ driver cannot be considered usable at the the present time.
To compile this driver as a module, choose M here: the module
will be called ipg. This is recommended.
Or should it be demoted to BROKEN? It compiles, and sends and receives
packets, which is better than a lot of BROKEN drivers.
^ permalink raw reply related
* Re: [PACKET]: Fix sparse warnings in af_packet.c
From: David Miller @ 2008-01-08 6:40 UTC (permalink / raw)
To: dada1; +Cc: netdev
In-Reply-To: <20080107120108.06453951.dada1@cosmosbay.com>
From: Eric Dumazet <dada1@cosmosbay.com>
Date: Mon, 7 Jan 2008 12:01:08 +0100
> CHECK net/packet/af_packet.c
> net/packet/af_packet.c:1876:14: warning: context imbalance in 'packet_seq_start' - wrong count at exit
> net/packet/af_packet.c:1888:13: warning: context imbalance in 'packet_seq_stop' - unexpected unlock
>
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
Applied.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox