* [net-2.6 PATCH] igbvf: do not modify tx_queue_len on link speed change
From: Jeff Kirsher @ 2010-03-25 22:11 UTC (permalink / raw)
To: davem; +Cc: netdev, gospo, Franco Fichtner, Emil Tantilov, Jeff Kirsher
From: Emil Tantilov <emil.s.tantilov@intel.com>
Previously the driver tweaked txqueuelen to avoid false Tx hang reports seen at half duplex.
This had the effect of overriding user set values on link change/reset. Testing shows that
adjusting only the timeout factor is sufficient to prevent Tx hang reports at half duplex.
Based on e1000e patch by Franco Fichtner <franco@lastsummer.de>
CC: Franco Fichtner <franco@lastsummer.de>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/igbvf/igbvf.h | 1 -
drivers/net/igbvf/netdev.c | 11 +----------
2 files changed, 1 insertions(+), 11 deletions(-)
diff --git a/drivers/net/igbvf/igbvf.h b/drivers/net/igbvf/igbvf.h
index a1774b2..debeee2 100644
--- a/drivers/net/igbvf/igbvf.h
+++ b/drivers/net/igbvf/igbvf.h
@@ -198,7 +198,6 @@ struct igbvf_adapter {
struct igbvf_ring *tx_ring /* One per active queue */
____cacheline_aligned_in_smp;
- unsigned long tx_queue_len;
unsigned int restart_queue;
u32 txd_cmd;
diff --git a/drivers/net/igbvf/netdev.c b/drivers/net/igbvf/netdev.c
index a77afd8..b41037e 100644
--- a/drivers/net/igbvf/netdev.c
+++ b/drivers/net/igbvf/netdev.c
@@ -1304,8 +1304,6 @@ static void igbvf_configure_tx(struct igbvf_adapter *adapter)
/* enable Report Status bit */
adapter->txd_cmd |= E1000_ADVTXD_DCMD_RS;
-
- adapter->tx_queue_len = adapter->netdev->tx_queue_len;
}
/**
@@ -1524,7 +1522,6 @@ void igbvf_down(struct igbvf_adapter *adapter)
del_timer_sync(&adapter->watchdog_timer);
- netdev->tx_queue_len = adapter->tx_queue_len;
netif_carrier_off(netdev);
/* record the stats before reset*/
@@ -1857,21 +1854,15 @@ static void igbvf_watchdog_task(struct work_struct *work)
&adapter->link_duplex);
igbvf_print_link_info(adapter);
- /*
- * tweak tx_queue_len according to speed/duplex
- * and adjust the timeout factor
- */
- netdev->tx_queue_len = adapter->tx_queue_len;
+ /* adjust timeout factor according to speed/duplex */
adapter->tx_timeout_factor = 1;
switch (adapter->link_speed) {
case SPEED_10:
txb2b = 0;
- netdev->tx_queue_len = 10;
adapter->tx_timeout_factor = 16;
break;
case SPEED_100:
txb2b = 0;
- netdev->tx_queue_len = 100;
/* maybe add some timeout factor ? */
break;
}
^ permalink raw reply related
* Re: [net-2.6 PATCH] bonding: fix broken multicast with round-robin mode
From: Jay Vosburgh @ 2010-03-25 22:31 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: netdev, lhh, bonding-devel
In-Reply-To: <20100325214033.GA28741@gospo.rdu.redhat.com>
Andy Gospodarek <andy@greyhouse.net> wrote:
>Round-robin (mode 0) does nothing to ensure that any multicast traffic
>originally destined for the host will continue to arrive at the host when
>the link that sent the IGMP join or membership report goes down. One of
>the benefits of absolute round-robin transmit.
>
>Keeping track of subscribed multicast groups for each slave did not seem
>like a good use of resources, so I decided to simply send on the
>curr_active slave of the bond (typically the first enslaved device that
>is up). This makes failover management simple as IGMP membership
>reports only need to be sent when the curr_active_slave changes. I
>tested this patch and it appears to work as expected.
>
>Originally reported by Lon Hohberger <lhh@redhat.com>.
>
>Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
Seems reasonable, modulo a couple of minor things (see below).
I checked, and the link failover logic appears to maintain
curr_active_slave even for round robin mode, which, prior to this patch,
didn't use it.
>CC: Lon Hohberger <lhh@redhat.com>
>CC: Jay Vosburgh <fubar@us.ibm.com>
>
>---
> drivers/net/bonding/bond_main.c | 34 ++++++++++++++++++++++++++--------
> 1 files changed, 26 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 430c022..0b38455 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> write_lock_bh(&bond->curr_slave_lock);
> }
> }
>+
>+ /* resend IGMP joins since all were sent on curr_active_slave */
>+ if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
>+ bond_resend_igmp_join_requests(bond);
>+ }
> }
>
> /**
>@@ -4138,22 +4143,35 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
> struct bonding *bond = netdev_priv(bond_dev);
> struct slave *slave, *start_at;
> int i, slave_no, res = 1;
>+ struct iphdr *iph = ip_hdr(skb);
>
> read_lock(&bond->lock);
>
> if (!BOND_IS_OK(bond))
> goto out;
>-
> /*
>- * Concurrent TX may collide on rr_tx_counter; we accept that
>- * as being rare enough not to justify using an atomic op here
>+ * Start with the curr_active_slave that joined the bond as the
>+ * default for sending IGMP traffic. For failover purposes one
>+ * needs to maintain some consistency for the interface that will
>+ * send the join/membership reports. The curr_active_slave found
>+ * will send all of this type of traffic.
> */
>- slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>+ if ((skb->protocol == htons(ETH_P_IP)) &&
>+ (iph->protocol == htons(IPPROTO_IGMP))) {
>+ slave = bond->curr_active_slave;
Technically, this should acquire bond->curr_slave_lock for read
around the inspection of curr_active_slave.
I believe you'll also want a test for curr_active_slave == NULL,
and free the skb if so (or do something else). There's a race window in
bond_release: when releasing the curr_active_slave, the field is left
momentarily NULL with the bond unlocked. This occurs after the
bond_change_active_slave(bond, NULL) call, during the lock dance prior
to the call bond_select_active_slave:
bond_main.c:bond_release():
[...]
if (oldcurrent == slave)
bond_change_active_slave(bond, NULL);
[...]
if (oldcurrent == slave) {
/*
* Note that we hold RTNL over this sequence, so there
* is no concern that another slave add/remove event
* will interfere.
*/
write_unlock_bh(&bond->lock);
[ race window is here ]
read_lock(&bond->lock);
write_lock_bh(&bond->curr_slave_lock);
bond_select_active_slave(bond);
write_unlock_bh(&bond->curr_slave_lock);
read_unlock(&bond->lock);
write_lock_bh(&bond->lock);
}
I'm reasonably sure the other TX functions (that need to) will
handle the case that curr_active_slave is NULL.
>+ } else {
>+ /*
>+ * Concurrent TX may collide on rr_tx_counter; we accept
>+ * that as being rare enough not to justify using an
>+ * atomic op here.
>+ */
>+ slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>
>- bond_for_each_slave(bond, slave, i) {
>- slave_no--;
>- if (slave_no < 0)
>- break;
>+ bond_for_each_slave(bond, slave, i) {
>+ slave_no--;
>+ if (slave_no < 0)
>+ break;
>+ }
> }
>
> start_at = slave;
-J
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* Re: [PATCH 1/3] mfd: add support for Janz CMOD-IO PCI MODULbus Carrier Board
From: Samuel Ortiz @ 2010-03-25 22:59 UTC (permalink / raw)
To: Ira W. Snyder; +Cc: linux-kernel, netdev, socketcan-core
In-Reply-To: <20100319182209.GD13672@ovro.caltech.edu>
Hi Ira,
First of all, sorry for the late reply. Then my answers:
On Fri, Mar 19, 2010 at 11:22:09AM -0700, Ira W. Snyder wrote:
>
> > > +/*
> > > + * Subdevice Support
> > > + */
> > Please use the mfd-core API for building and registering platform sub devices.
> > The pieces of code below should shrink significantly.
> >
>
> Using this framework, how is it possible to create the devices that I
> do down below. For each subdevice, I need three resources:
>
> 1) MODULbus registers -- PCI BAR3 + (0x200 * module_num)
> 2) PLX Control Registers -- PCI BAR4
> 3) IRQ
>
> Specifically, the way IORESOURCE_MEM resources are copied seems wrong.
> They start at the base address of only one resource and use the offsets
> provided in the struct mfd_cell. See the if-statement at
> drivers/mfd/mfd-core.c line 48.
>
> I need two use two different parent resources. The mfd_add_devices()
> function doesn't support this.
I would still like you to use the mfd-core API. Here is my proposal:
1) I modify mfd_add_device() to support a NULL mem_base argument. When
mem_base is NULL, we would have:
res[r].parent = NULL and res[r].start = cell->resources[r].start;
The platform code will use iomem_resource as the parent for this resource.
2) Your mfd_cell cells would have 3 resources, and you just need to set the
IORESOURCE_MEM ones at probe time, with pci->resource[n]->start + offset as
the start field.
Would that make sense to you ?
> > > + /* Onboard configuration registers */
> > > + priv->ctrl = pci_ioremap_bar(dev, 4);
> > Why 4 ?
> >
> >
>
> Because that is how the device works ;) There is a comment up above that
> describes them as the "PLX control registers". Are you suggesting that I
> add a comment here too?
No, that's ok, I missed the comment.
> > > +#define PCI_VENDOR_ID_JANZ 0x13c3
> > That probably belongs to pci_ids.h
> >
>
> Should I add a patch to the series for this?
Either that or merge the pci_ids.h changes with this patch.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
^ permalink raw reply
* [PATCH] igb: update hw_debug macro to make use of netdev_dbg call
From: Jeff Kirsher @ 2010-03-25 23:10 UTC (permalink / raw)
To: davem; +Cc: netdev, gospo, Alexander Duyck, Jeff Kirsher
From: Alexander Duyck <alexander.h.duyck@intel.com>
This change updates the igb driver to make use of the netdev_dbg function
macros now provided in netdevice.h
This is meant to be provided as an alternative to the patch provided by
Joe Perches.
It also removes igb_get_time_str since I found that it is unused code that
is no longer used even in debug.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/igb/e1000_hw.h | 12 +++++-------
drivers/net/igb/igb_main.c | 32 +++-----------------------------
2 files changed, 8 insertions(+), 36 deletions(-)
diff --git a/drivers/net/igb/e1000_hw.h b/drivers/net/igb/e1000_hw.h
index 82a533f..fbaff4d 100644
--- a/drivers/net/igb/e1000_hw.h
+++ b/drivers/net/igb/e1000_hw.h
@@ -31,6 +31,7 @@
#include <linux/types.h>
#include <linux/delay.h>
#include <linux/io.h>
+#include <linux/netdevice.h>
#include "e1000_regs.h"
#include "e1000_defines.h"
@@ -502,14 +503,11 @@ struct e1000_hw {
u8 revision_id;
};
-#ifdef DEBUG
-extern char *igb_get_hw_dev_name(struct e1000_hw *hw);
+extern struct net_device *igb_get_hw_dev(struct e1000_hw *hw);
#define hw_dbg(format, arg...) \
- printk(KERN_DEBUG "%s: " format, igb_get_hw_dev_name(hw), ##arg)
-#else
-#define hw_dbg(format, arg...)
-#endif
-#endif
+ netdev_dbg(igb_get_hw_dev(hw), format, ##arg)
+
/* These functions must be implemented by drivers */
s32 igb_read_pcie_cap_reg(struct e1000_hw *hw, u32 reg, u16 *value);
s32 igb_write_pcie_cap_reg(struct e1000_hw *hw, u32 reg, u16 *value);
+#endif /* _E1000_HW_H_ */
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 01c65c7..1e4d5a6 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -222,43 +222,17 @@ static cycle_t igb_read_clock(const struct cyclecounter *tc)
return stamp;
}
-#ifdef DEBUG
/**
- * igb_get_hw_dev_name - return device name string
+ * igb_get_hw_dev - return device
* used by hardware layer to print debugging information
**/
-char *igb_get_hw_dev_name(struct e1000_hw *hw)
+struct net_device *igb_get_hw_dev(struct e1000_hw *hw)
{
struct igb_adapter *adapter = hw->back;
- return adapter->netdev->name;
+ return adapter->netdev;
}
/**
- * igb_get_time_str - format current NIC and system time as string
- */
-static char *igb_get_time_str(struct igb_adapter *adapter,
- char buffer[160])
-{
- cycle_t hw = adapter->cycles.read(&adapter->cycles);
- struct timespec nic = ns_to_timespec(timecounter_read(&adapter->clock));
- struct timespec sys;
- struct timespec delta;
- getnstimeofday(&sys);
-
- delta = timespec_sub(nic, sys);
-
- sprintf(buffer,
- "HW %llu, NIC %ld.%09lus, SYS %ld.%09lus, NIC-SYS %lds + %09luns",
- hw,
- (long)nic.tv_sec, nic.tv_nsec,
- (long)sys.tv_sec, sys.tv_nsec,
- (long)delta.tv_sec, delta.tv_nsec);
-
- return buffer;
-}
-#endif
-
-/**
* igb_init_module - Driver Registration Routine
*
* igb_init_module is the first routine called when the driver is
^ permalink raw reply related
* Re: [PATCH 1/3] mfd: add support for Janz CMOD-IO PCI MODULbus Carrier Board
From: Ira W. Snyder @ 2010-03-25 23:22 UTC (permalink / raw)
To: Samuel Ortiz
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20100325225929.GA20618-jcdQHdrhKHMdnm+yROfE0A@public.gmane.org>
On Thu, Mar 25, 2010 at 11:59:30PM +0100, Samuel Ortiz wrote:
> Hi Ira,
>
> First of all, sorry for the late reply. Then my answers:
>
> On Fri, Mar 19, 2010 at 11:22:09AM -0700, Ira W. Snyder wrote:
> >
> > > > +/*
> > > > + * Subdevice Support
> > > > + */
> > > Please use the mfd-core API for building and registering platform sub devices.
> > > The pieces of code below should shrink significantly.
> > >
> >
> > Using this framework, how is it possible to create the devices that I
> > do down below. For each subdevice, I need three resources:
> >
> > 1) MODULbus registers -- PCI BAR3 + (0x200 * module_num)
> > 2) PLX Control Registers -- PCI BAR4
> > 3) IRQ
> >
> > Specifically, the way IORESOURCE_MEM resources are copied seems wrong.
> > They start at the base address of only one resource and use the offsets
> > provided in the struct mfd_cell. See the if-statement at
> > drivers/mfd/mfd-core.c line 48.
> >
> > I need two use two different parent resources. The mfd_add_devices()
> > function doesn't support this.
> I would still like you to use the mfd-core API. Here is my proposal:
>
> 1) I modify mfd_add_device() to support a NULL mem_base argument. When
> mem_base is NULL, we would have:
>
> res[r].parent = NULL and res[r].start = cell->resources[r].start;
>
> The platform code will use iomem_resource as the parent for this resource.
>
I don't know the implications of using iomem_resource as the parent
resource. If you think it is ok, I have no objections.
If it helps, I can provide the PCI resource as a parent resource in my
resources. Then, when mem_base is NULL, the mfd-core could do this:
res[r].parent = cell->resources[r].parent
This is basically what I did in my patch. I used the PCI resource as the
parent of all child resources. I don't know if that is safe, but it
works. :)
The mfd_add_device() function does this for IORESOURCE_IO resources. It
only tries to be smart for IORESOURCE_MEM and IORESOURCE_IRQ resources.
> 2) Your mfd_cell cells would have 3 resources, and you just need to set the
> IORESOURCE_MEM ones at probe time, with pci->resource[n]->start + offset as
> the start field.
>
> Would that make sense to you ?
>
Yep, that makes sense.
> > > > + /* Onboard configuration registers */
> > > > + priv->ctrl = pci_ioremap_bar(dev, 4);
> > > Why 4 ?
> > >
> > >
> >
> > Because that is how the device works ;) There is a comment up above that
> > describes them as the "PLX control registers". Are you suggesting that I
> > add a comment here too?
> No, that's ok, I missed the comment.
>
> > > > +#define PCI_VENDOR_ID_JANZ 0x13c3
> > > That probably belongs to pci_ids.h
> > >
> >
> > Should I add a patch to the series for this?
> Either that or merge the pci_ids.h changes with this patch.
>
I guess it is a trivial enough change to merge with this patch.
I'll wait for your patch to the mfd-core API before making changes and
sending out the next round of updates.
Thanks for replying,
Ira
^ permalink raw reply
* Re: [PATCH 1/3] mfd: add support for Janz CMOD-IO PCI MODULbus Carrier Board
From: Samuel Ortiz @ 2010-03-26 0:26 UTC (permalink / raw)
To: Ira W. Snyder; +Cc: linux-kernel, netdev, socketcan-core
In-Reply-To: <20100325232244.GG10454@ovro.caltech.edu>
On Thu, Mar 25, 2010 at 04:22:44PM -0700, Ira W. Snyder wrote:
> On Thu, Mar 25, 2010 at 11:59:30PM +0100, Samuel Ortiz wrote:
> > Hi Ira,
> >
> > First of all, sorry for the late reply. Then my answers:
> >
> > On Fri, Mar 19, 2010 at 11:22:09AM -0700, Ira W. Snyder wrote:
> > >
> > > > > +/*
> > > > > + * Subdevice Support
> > > > > + */
> > > > Please use the mfd-core API for building and registering platform sub devices.
> > > > The pieces of code below should shrink significantly.
> > > >
> > >
> > > Using this framework, how is it possible to create the devices that I
> > > do down below. For each subdevice, I need three resources:
> > >
> > > 1) MODULbus registers -- PCI BAR3 + (0x200 * module_num)
> > > 2) PLX Control Registers -- PCI BAR4
> > > 3) IRQ
> > >
> > > Specifically, the way IORESOURCE_MEM resources are copied seems wrong.
> > > They start at the base address of only one resource and use the offsets
> > > provided in the struct mfd_cell. See the if-statement at
> > > drivers/mfd/mfd-core.c line 48.
> > >
> > > I need two use two different parent resources. The mfd_add_devices()
> > > function doesn't support this.
> > I would still like you to use the mfd-core API. Here is my proposal:
> >
> > 1) I modify mfd_add_device() to support a NULL mem_base argument. When
> > mem_base is NULL, we would have:
> >
> > res[r].parent = NULL and res[r].start = cell->resources[r].start;
> >
> > The platform code will use iomem_resource as the parent for this resource.
> >
>
> I don't know the implications of using iomem_resource as the parent
> resource. If you think it is ok, I have no objections.
>
> If it helps, I can provide the PCI resource as a parent resource in my
> resources. Then, when mem_base is NULL, the mfd-core could do this:
>
> res[r].parent = cell->resources[r].parent
>
> This is basically what I did in my patch. I used the PCI resource as the
> parent of all child resources. I don't know if that is safe, but it
> works. :)
>
> The mfd_add_device() function does this for IORESOURCE_IO resources. It
> only tries to be smart for IORESOURCE_MEM and IORESOURCE_IRQ resources.
I pushed an mfd-core change that basically falls back to the default resource
copying when mem_base is NULL. That should allow you to use the API now.
> > > > > +#define PCI_VENDOR_ID_JANZ 0x13c3
> > > > That probably belongs to pci_ids.h
> > > >
> > >
> > > Should I add a patch to the series for this?
> > Either that or merge the pci_ids.h changes with this patch.
> >
>
> I guess it is a trivial enough change to merge with this patch.
>
> I'll wait for your patch to the mfd-core API before making changes and
> sending out the next round of updates.
Very nice. The above mentioned change in my for-next branch, commit
6802a325f541bbea3168cf61ba239443193e1f9a.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
^ permalink raw reply
* UDP path MTU discovery
From: Glen Turner @ 2010-03-26 0:02 UTC (permalink / raw)
To: netdev
[This is a second attempt to report this bug.]
Path MTU Discovery for UDP underperforms for IPv4 and fails
for IPv6 in Linux for transactional services like DHCP and
RADIUS running on jumbo frame interfaces.
These servers send packets with exponential back-off. UDP
Path MTU Discovery probes for the path MTU each time the
application sends a packet. So if you start with a high
enough interface MTU then the server application backoff
times get huge and the client gives up before the path
MTU is discovered.
This differs from TCP, where it is the kernel -- and not
the application -- which organises retransmission. On
receiving a ICMP Fragmentation Needed the kernel can
immediately re-probe the path MTU wiht no waiting for
an exponential timer to expire.
In IPv4 there is a work-around for the server, turn off
Path MTU Discovery and allow routers to fragment the packet
as needed. Looking at the code for the various transactional
servers (ISC DHCP, FreeRADIUS, RADIATOR, radsecproxy) they
all disable Path MTU Discovery on Linux. This workaround has
the side effect of hiding the problem, misleading people into
thinking that UDP Path MTU Discovery actually works for these
transactional servers.
In IPv6 routers do not fragment packets, so there is no work
around. Transactional servers which use UDP over IPv6 encounter
exponential backoffs within the application and the client
abandons the transaction. There is no way for the server to
know that the packet was lost due to Path MTU Discovery and
to immediately re-transmit it (without an exponential penalty)
so that the MTU can be probed again.
This can be viewed as a flaw in the RFC and in the sockets API
for which IPv6 has removed the common work-around.
Thank you, Glen
--
Glen Turner
www.gdt.id.au/~gdt
^ permalink raw reply
* Re: [net-2.6 PATCH v2] bonding: fix broken multicast with round-robin mode
From: Andy Gospodarek @ 2010-03-26 0:49 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, lhh, bonding-devel
In-Reply-To: <24080.1269556271@death.nxdomain.ibm.com>
On Thu, Mar 25, 2010 at 03:31:11PM -0700, Jay Vosburgh wrote:
> Andy Gospodarek <andy@greyhouse.net> wrote:
>
> >Round-robin (mode 0) does nothing to ensure that any multicast traffic
> >originally destined for the host will continue to arrive at the host when
> >the link that sent the IGMP join or membership report goes down. One of
> >the benefits of absolute round-robin transmit.
> >
> >Keeping track of subscribed multicast groups for each slave did not seem
> >like a good use of resources, so I decided to simply send on the
> >curr_active slave of the bond (typically the first enslaved device that
> >is up). This makes failover management simple as IGMP membership
> >reports only need to be sent when the curr_active_slave changes. I
> >tested this patch and it appears to work as expected.
> >
> >Originally reported by Lon Hohberger <lhh@redhat.com>.
> >
> >Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>
> Seems reasonable, modulo a couple of minor things (see below).
>
> I checked, and the link failover logic appears to maintain
> curr_active_slave even for round robin mode, which, prior to this patch,
> didn't use it.
Correct. I initially didn't plan to use that, but when I saw that it
was maintained for round-robin mode as well I decided it would be good
to use it rather than wasting space with another pointer.
>
> >CC: Lon Hohberger <lhh@redhat.com>
> >CC: Jay Vosburgh <fubar@us.ibm.com>
> >
> >---
> > drivers/net/bonding/bond_main.c | 34 ++++++++++++++++++++++++++--------
> > 1 files changed, 26 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> >index 430c022..0b38455 100644
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> > write_lock_bh(&bond->curr_slave_lock);
> > }
> > }
> >+
> >+ /* resend IGMP joins since all were sent on curr_active_slave */
> >+ if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
> >+ bond_resend_igmp_join_requests(bond);
> >+ }
> > }
> >
> > /**
> >@@ -4138,22 +4143,35 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
> > struct bonding *bond = netdev_priv(bond_dev);
> > struct slave *slave, *start_at;
> > int i, slave_no, res = 1;
> >+ struct iphdr *iph = ip_hdr(skb);
> >
> > read_lock(&bond->lock);
> >
> > if (!BOND_IS_OK(bond))
> > goto out;
> >-
> > /*
> >- * Concurrent TX may collide on rr_tx_counter; we accept that
> >- * as being rare enough not to justify using an atomic op here
> >+ * Start with the curr_active_slave that joined the bond as the
> >+ * default for sending IGMP traffic. For failover purposes one
> >+ * needs to maintain some consistency for the interface that will
> >+ * send the join/membership reports. The curr_active_slave found
> >+ * will send all of this type of traffic.
> > */
> >- slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
> >+ if ((skb->protocol == htons(ETH_P_IP)) &&
> >+ (iph->protocol == htons(IPPROTO_IGMP))) {
> >+ slave = bond->curr_active_slave;
>
> Technically, this should acquire bond->curr_slave_lock for read
> around the inspection of curr_active_slave.
>
> I believe you'll also want a test for curr_active_slave == NULL,
> and free the skb if so (or do something else). There's a race window in
> bond_release: when releasing the curr_active_slave, the field is left
> momentarily NULL with the bond unlocked. This occurs after the
> bond_change_active_slave(bond, NULL) call, during the lock dance prior
> to the call bond_select_active_slave:
>
> bond_main.c:bond_release():
> [...]
> if (oldcurrent == slave)
> bond_change_active_slave(bond, NULL);
> [...]
> if (oldcurrent == slave) {
> /*
> * Note that we hold RTNL over this sequence, so there
> * is no concern that another slave add/remove event
> * will interfere.
> */
> write_unlock_bh(&bond->lock);
>
> [ race window is here ]
>
> read_lock(&bond->lock);
> write_lock_bh(&bond->curr_slave_lock);
>
> bond_select_active_slave(bond);
>
> write_unlock_bh(&bond->curr_slave_lock);
> read_unlock(&bond->lock);
> write_lock_bh(&bond->lock);
> }
>
> I'm reasonably sure the other TX functions (that need to) will
> handle the case that curr_active_slave is NULL.
>
Good catch, Jay. Thanks for looking at this.
Here is an updated and tested patch:
[net-2.6 PATCH] bonding: fix broken multicast with round-robin mode
Round-robin (mode 0) does nothing to ensure that any multicast traffic
originally destined for the host will continue to arrive at the host when
the link that sent the IGMP join or membership report goes down. One of
the benefits of absolute round-robin transmit.
Keeping track of subscribed multicast groups for each slave did not seem
like a good use of resources, so I decided to simply send on the
curr_active slave of the bond (typically the first enslaved device that
is up). This makes failover management simple as IGMP membership
reports only need to be sent when the curr_active_slave changes. I
tested this patch and it appears to work as expected.
Originally reported by Lon Hohberger <lhh@redhat.com>.
Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
CC: Lon Hohberger <lhh@redhat.com>
CC: Jay Vosburgh <fubar@us.ibm.com>
---
drivers/net/bonding/bond_main.c | 40 +++++++++++++++++++++++++++++++-------
1 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 430c022..5b92fbf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
write_lock_bh(&bond->curr_slave_lock);
}
}
+
+ /* resend IGMP joins since all were sent on curr_active_slave */
+ if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
+ bond_resend_igmp_join_requests(bond);
+ }
}
/**
@@ -4138,22 +4143,41 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
struct bonding *bond = netdev_priv(bond_dev);
struct slave *slave, *start_at;
int i, slave_no, res = 1;
+ struct iphdr *iph = ip_hdr(skb);
read_lock(&bond->lock);
if (!BOND_IS_OK(bond))
goto out;
-
/*
- * Concurrent TX may collide on rr_tx_counter; we accept that
- * as being rare enough not to justify using an atomic op here
+ * Start with the curr_active_slave that joined the bond as the
+ * default for sending IGMP traffic. For failover purposes one
+ * needs to maintain some consistency for the interface that will
+ * send the join/membership reports. The curr_active_slave found
+ * will send all of this type of traffic.
*/
- slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
+ if ((iph->protocol == htons(IPPROTO_IGMP)) &&
+ (skb->protocol == htons(ETH_P_IP))) {
- bond_for_each_slave(bond, slave, i) {
- slave_no--;
- if (slave_no < 0)
- break;
+ read_lock(&bond->curr_slave_lock);
+ slave = bond->curr_active_slave;
+ read_unlock(&bond->curr_slave_lock);
+
+ if (!slave)
+ goto out;
+ } else {
+ /*
+ * Concurrent TX may collide on rr_tx_counter; we accept
+ * that as being rare enough not to justify using an
+ * atomic op here.
+ */
+ slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
+
+ bond_for_each_slave(bond, slave, i) {
+ slave_no--;
+ if (slave_no < 0)
+ break;
+ }
}
start_at = slave;
^ permalink raw reply related
* Re: UDP path MTU discovery
From: Rick Jones @ 2010-03-26 0:53 UTC (permalink / raw)
To: Glen Turner; +Cc: netdev
In-Reply-To: <1269561751.2891.8.camel@ilion>
Glen Turner wrote:
> [This is a second attempt to report this bug.]
Bugzilla somewhere?
> Path MTU Discovery for UDP underperforms for IPv4 and fails for IPv6 in Linux
> for transactional services like DHCP and RADIUS running on jumbo frame
> interfaces.
>
> These servers send packets with exponential back-off. UDP Path MTU Discovery
> probes for the path MTU each time the application sends a packet. So if you
> start with a high enough interface MTU then the server application backoff
> times get huge and the client gives up before the path MTU is discovered.
>
> This differs from TCP, where it is the kernel -- and not the application --
> which organises retransmission. On receiving a ICMP Fragmentation Needed the
> kernel can immediately re-probe the path MTU wiht no waiting for an
> exponential timer to expire.
>
> In IPv4 there is a work-around for the server, turn off Path MTU Discovery
> and allow routers to fragment the packet as needed. Looking at the code for
> the various transactional servers (ISC DHCP, FreeRADIUS, RADIATOR,
> radsecproxy) they all disable Path MTU Discovery on Linux. This workaround
> has the side effect of hiding the problem, misleading people into thinking
> that UDP Path MTU Discovery actually works for these transactional servers.
>
> In IPv6 routers do not fragment packets, so there is no work around.
> Transactional servers which use UDP over IPv6 encounter exponential backoffs
> within the application and the client abandons the transaction. There is no
> way for the server to know that the packet was lost due to Path MTU Discovery
> and to immediately re-transmit it (without an exponential penalty) so that
> the MTU can be probed again.
>
> This can be viewed as a flaw in the RFC and in the sockets API
> for which IPv6 has removed the common work-around.
So, presuming it is indeed a bug what form might a fix take? Are you suggesting
there should be a way for an application to say "Please let me see/know about
the ICMP messages?" Is that option available on other platforms as a
platform-specific extension? I don't have the details, but the HP-UX 11i v3
(11.31) netinet/udp.h file contains these:
#define UDP_RX_ICMP 0x02 /* boolean; get/set ICMP packets reception */
/* Set to 1 if ICMP packets are to be received*/
#define UDP_RX_ICMP6 0x03 /* boolean; get/set ICMPv6 packets reception */
/* Set to 1 if ICMPv6 packets are to be
received */
and it does appear that they are in more places than just HP-UX - there are some
hits for that for the old Apple Open Transport - which makes sense - it too had
Mentat origins.
rick jones
^ permalink raw reply
* Re: [net-2.6 PATCH v2] bonding: fix broken multicast with round-robin mode
From: Jay Vosburgh @ 2010-03-26 0:55 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: netdev, lhh, bonding-devel
In-Reply-To: <20100326004905.GB28741@gospo.rdu.redhat.com>
Andy Gospodarek <andy@greyhouse.net> wrote:
[...]
>Here is an updated and tested patch:
>
>[net-2.6 PATCH] bonding: fix broken multicast with round-robin mode
>
>Round-robin (mode 0) does nothing to ensure that any multicast traffic
>originally destined for the host will continue to arrive at the host when
>the link that sent the IGMP join or membership report goes down. One of
>the benefits of absolute round-robin transmit.
>
>Keeping track of subscribed multicast groups for each slave did not seem
>like a good use of resources, so I decided to simply send on the
>curr_active slave of the bond (typically the first enslaved device that
>is up). This makes failover management simple as IGMP membership
>reports only need to be sent when the curr_active_slave changes. I
>tested this patch and it appears to work as expected.
>
>Originally reported by Lon Hohberger <lhh@redhat.com>.
>
>Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
>CC: Lon Hohberger <lhh@redhat.com>
>CC: Jay Vosburgh <fubar@us.ibm.com>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
>---
> drivers/net/bonding/bond_main.c | 40 +++++++++++++++++++++++++++++++-------
> 1 files changed, 32 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 430c022..5b92fbf 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1235,6 +1235,11 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> write_lock_bh(&bond->curr_slave_lock);
> }
> }
>+
>+ /* resend IGMP joins since all were sent on curr_active_slave */
>+ if (bond->params.mode == BOND_MODE_ROUNDROBIN) {
>+ bond_resend_igmp_join_requests(bond);
>+ }
> }
>
> /**
>@@ -4138,22 +4143,41 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
> struct bonding *bond = netdev_priv(bond_dev);
> struct slave *slave, *start_at;
> int i, slave_no, res = 1;
>+ struct iphdr *iph = ip_hdr(skb);
>
> read_lock(&bond->lock);
>
> if (!BOND_IS_OK(bond))
> goto out;
>-
> /*
>- * Concurrent TX may collide on rr_tx_counter; we accept that
>- * as being rare enough not to justify using an atomic op here
>+ * Start with the curr_active_slave that joined the bond as the
>+ * default for sending IGMP traffic. For failover purposes one
>+ * needs to maintain some consistency for the interface that will
>+ * send the join/membership reports. The curr_active_slave found
>+ * will send all of this type of traffic.
> */
>- slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>+ if ((iph->protocol == htons(IPPROTO_IGMP)) &&
>+ (skb->protocol == htons(ETH_P_IP))) {
>
>- bond_for_each_slave(bond, slave, i) {
>- slave_no--;
>- if (slave_no < 0)
>- break;
>+ read_lock(&bond->curr_slave_lock);
>+ slave = bond->curr_active_slave;
>+ read_unlock(&bond->curr_slave_lock);
>+
>+ if (!slave)
>+ goto out;
>+ } else {
>+ /*
>+ * Concurrent TX may collide on rr_tx_counter; we accept
>+ * that as being rare enough not to justify using an
>+ * atomic op here.
>+ */
>+ slave_no = bond->rr_tx_counter++ % bond->slave_cnt;
>+
>+ bond_for_each_slave(bond, slave, i) {
>+ slave_no--;
>+ if (slave_no < 0)
>+ break;
>+ }
> }
>
> start_at = slave;
^ permalink raw reply
* Re: [Regression] r8169: enable 64-bit DMA by default for PCI Express devices (v2)
From: Robert Hancock @ 2010-03-26 0:56 UTC (permalink / raw)
To: Jörn Engel; +Cc: David Miller, torvalds, linux-kernel, netdev, romieu
In-Reply-To: <51f3faa71003161630g69160ea9tc1a2d448682632e5@mail.gmail.com>
On Tue, Mar 16, 2010 at 5:30 PM, Robert Hancock <hancockrwd@gmail.com> wrote:
> On Tue, Mar 16, 2010 at 2:35 AM, Jörn Engel <joern@dublin.logfs.org> wrote:
>> On Mon, 15 March 2010 17:28:45 -0600, Robert Hancock wrote:
>>>
>>> What are the symptoms?
>>
>> No more network. :)
>>
>> According to ifconfig the interface is up and running. If I read
>> /sys/class/net/eth0 correctly, two packets have been sent and none
>> received. LED on network card and switch is off. mii-tool is
>> unhappy.
>>
>> With patch in:
>> Bikini:~# mii-tool
>> No MII transceiver present!.
>>
>> With patch reverted:
>> Bikini:~# mii-tool
>> eth0: negotiated 100baseTx-FD flow-control, link ok
>>
>> I just noticed lspci is also unhappy.
>>
>> With patch in:
>> Bikini:~# lspci -vv > lspci
>> pcilib: sysfs_read_vpd: read failed: Connection timed out.
>>
>> With patch reverted:
>> Bikini:~# lspci -vv > lspci2
>> Bikini:~# diff -u lspci*
>> --- lspci 2010-03-16 09:03:02.000000000 +0100
>> +++ lspci2 2010-03-16 09:09:36.000000000 +0100
>> @@ -246,7 +246,7 @@
>> Vector table: BAR=4 offset=00000000
>> PBA: BAR=4 offset=00000800
>> Capabilities: [cc] Vital Product Data
>> - Not readable
>> + Unknown small resource type 00, will not decode more.
>> Capabilities: [100] Advanced Error Reporting
>> UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
>> UEMsk: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
>>
>> As you might expect, delta is part of the ethernet controller.
>>
>> All "With patch in" output above is typed off the screen. Beware of
>> typos.
>>
>>> Does setting use_dac=0 in the module options
>>> for r8169 also resolve the problem?
>>
>> /sys/modules/r8169 does not exist. How odd. Anyway, simply changing
>> the default in r8169.c to 0 fixes the problems for me. Good call.
>
>
> I can't really explain how the MII and lspci output is being
> affected.. However, it's possible that some of the chips supported by
> this driver don't actually support 64-bit DMA. Your chip is quite a
> bit older than mine (RTL8102E seems to be one of the older, if not the
> oldest, PCI-E chipset supported by the driver).
>
> Francois, do you know if that's the case, or do you have any active
> contacts at Realtek who can say?
>
> In the meantime, I don't object to reverting the patch for now unless
> we get this sorted out quickly..
Francois, ping? Is there anyone else that has access to this kind of
information about these chips?
It's kind of interesting that there's only been one report of this
though. Either the affected chips are rare among people testing
2.6.34-rc or there's something more to this. Maybe something
wierd/unusual about Jörn's system?
Jörn, are any other devices on your system working with 64-bit
addressing? Try doing this:
find /sys -name "*dma_mask_bits*" | xargs cat
Does anything show more than 32?
^ permalink raw reply
* Re: [PATCH RFC] fix problems with NETIF_F_HIGHDMA in networking drivers v2
From: Robert Hancock @ 2010-03-26 1:03 UTC (permalink / raw)
To: FUJITA Tomonori; +Cc: linux-kernel, netdev, linux-usb, davem, bzolnier
In-Reply-To: <20100304135738C.fujita.tomonori@lab.ntt.co.jp>
On Wed, Mar 3, 2010 at 10:58 PM, FUJITA Tomonori
<fujita.tomonori@lab.ntt.co.jp> wrote:
> On Wed, 03 Mar 2010 20:55:55 -0600
> Robert Hancock <hancockrwd@gmail.com> wrote:
>
>> Many networking drivers have issues with the use of the NETIF_F_HIGHDMA flag.
>> This flag actually indicates whether or not the device/driver can handle
>> skbs located in high memory (as opposed to lowmem). If the flag isn't set and
>> the skb is located in highmem, it needs to be copied.
>> There are two problems with this flag:
>>
>> -Many drivers only set the flag when they detect they can use 64-bit DMA,
>> since otherwise they could receive DMA addresses that they can't handle
>> (which on platforms without IOMMU/SWIOTLB support is fatal). This means that if
>> 64-bit support isn't available, even buffers located below 4GB will get copied
>> unnecessarily.
>>
>> -Some drivers set the flag even though they can't actually handle 64-bit DMA,
>> which would mean that on platforms without IOMMU/SWIOTLB they would get a DMA
>> mapping error if the memory they received happened to be located above 4GB.
>>
>> In order to fix this problem, the existing NETIF_F_HIGHDMA flag is split into
>> two new flags:
>>
>> NETIF_F_DMA_HIGH - indicates if the driver can do DMA to highmem at all
>> NETIF_F_DMA_64BIT - indicates the driver can do DMA to 64-bit memory
>
> Why can't you use dev->dma_mask here like the following?
>
> Then you can fix drivers that use the NETIF_F_HIGHDMA flag to indicate
> that they don't support 64bit DMA.
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index bcc490c..b15f94b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -129,6 +129,7 @@
> #include <linux/jhash.h>
> #include <linux/random.h>
> #include <trace/events/napi.h>
> +#include <linux/pci.h>
>
> #include "net-sysfs.h"
>
> @@ -1787,14 +1788,21 @@ static inline int illegal_highdma(struct net_device *dev, struct sk_buff *skb)
> {
> #ifdef CONFIG_HIGHMEM
> int i;
> + if (!(dev->features & NETIF_F_HIGHDMA)) {
> + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> + if (PageHighMem(skb_shinfo(skb)->frags[i].page))
> + return 1;
> + }
>
> - if (dev->features & NETIF_F_HIGHDMA)
> - return 0;
> -
> - for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> - if (PageHighMem(skb_shinfo(skb)->frags[i].page))
> - return 1;
> + if (PCI_DMA_BUS_IS_PHYS) {
> + struct device *pdev = dev->dev.parent;
>
> + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
> + dma_addr_t addr = page_to_phys(skb_shinfo(skb)->frags[i].page);
> + if (!pdev->dma_mask || addr + PAGE_SIZE - 1 > *pdev->dma_mask)
> + return 1;
> + }
> + }
> #endif
> return 0;
> }
>
This seems like it could be a reasonable approach. The only thing is
that in this code you're returning 1 if the parent device has no DMA
mask set. Wouldn't it make more sense to return 0 in this case? I'm
assuming that in that situation it's a virtual device not backed by
any hardware and there should be no DMA mask restriction...
^ permalink raw reply
* [net-2.6 PATCH] ixgbe: Do not run all Diagnostic offline tests when VFs are active
From: Jeff Kirsher @ 2010-03-26 3:06 UTC (permalink / raw)
To: davem; +Cc: netdev, gospo, Greg Rose, Jeff Kirsher
From: Greg Rose <gregory.v.rose@intel.com>
When running the offline diagnostic tests check to see if any VFs are
online. If so then only run the link test. This is necessary because
the VFs running in guest VMs aren't aware of when the PF is taken
offline for a diagnostic test. Also put a message to the system log
telling the system administrator to take the VFs offline manually if
(s)he wants to run a full diagnostic. Return 1 on each of the tests
not run to alert the user of the condition.
Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ixgbe/ixgbe_ethtool.c | 21 +++++++++++++++++++++
1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_ethtool.c b/drivers/net/ixgbe/ixgbe_ethtool.c
index 7949a44..1959ef7 100644
--- a/drivers/net/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ixgbe/ixgbe_ethtool.c
@@ -1853,6 +1853,26 @@ static void ixgbe_diag_test(struct net_device *netdev,
if (ixgbe_link_test(adapter, &data[4]))
eth_test->flags |= ETH_TEST_FL_FAILED;
+ if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) {
+ int i;
+ for (i = 0; i < adapter->num_vfs; i++) {
+ if (adapter->vfinfo[i].clear_to_send) {
+ netdev_warn(netdev, "%s",
+ "offline diagnostic is not "
+ "supported when VFs are "
+ "present\n");
+ data[0] = 1;
+ data[1] = 1;
+ data[2] = 1;
+ data[3] = 1;
+ eth_test->flags |= ETH_TEST_FL_FAILED;
+ clear_bit(__IXGBE_TESTING,
+ &adapter->state);
+ goto skip_ol_tests;
+ }
+ }
+ }
+
if (if_running)
/* indicate we're in test mode */
dev_close(netdev);
@@ -1908,6 +1928,7 @@ skip_loopback:
clear_bit(__IXGBE_TESTING, &adapter->state);
}
+skip_ol_tests:
msleep_interruptible(4 * 1000);
}
^ permalink raw reply related
* [net-2.6 PATCH] igb: use correct bits to identify if managability is enabled
From: Jeff Kirsher @ 2010-03-26 3:15 UTC (permalink / raw)
To: davem; +Cc: netdev, gospo, Alexander Duyck, Jeff Kirsher
From: Alexander Duyck <alexander.h.duyck@intel.com>
igb was previously checking the wrong bits in the MANC register to determine
if managability was enabled. As a result it was incorrectly powering down and
resetting the phy when it didn't need to.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/igb/e1000_mac.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/igb/e1000_mac.c b/drivers/net/igb/e1000_mac.c
index 2a8a886..be8d010 100644
--- a/drivers/net/igb/e1000_mac.c
+++ b/drivers/net/igb/e1000_mac.c
@@ -1367,7 +1367,8 @@ out:
* igb_enable_mng_pass_thru - Enable processing of ARP's
* @hw: pointer to the HW structure
*
- * Verifies the hardware needs to allow ARPs to be processed by the host.
+ * Verifies the hardware needs to leave interface enabled so that frames can
+ * be directed to and from the management interface.
**/
bool igb_enable_mng_pass_thru(struct e1000_hw *hw)
{
@@ -1380,8 +1381,7 @@ bool igb_enable_mng_pass_thru(struct e1000_hw *hw)
manc = rd32(E1000_MANC);
- if (!(manc & E1000_MANC_RCV_TCO_EN) ||
- !(manc & E1000_MANC_EN_MAC_ADDR_FILTER))
+ if (!(manc & E1000_MANC_RCV_TCO_EN))
goto out;
if (hw->mac.arc_subsystem_valid) {
^ permalink raw reply related
* Re: UDP path MTU discovery
From: David Miller @ 2010-03-26 3:24 UTC (permalink / raw)
To: gdt; +Cc: netdev
In-Reply-To: <1269561751.2891.8.camel@ilion>
From: Glen Turner <gdt@gdt.id.au>
Date: Fri, 26 Mar 2010 10:32:31 +1030
> This differs from TCP, where it is the kernel -- and not
> the application -- which organises retransmission. On
> receiving a ICMP Fragmentation Needed the kernel can
> immediately re-probe the path MTU wiht no waiting for
> an exponential timer to expire.
So the argument is, the kernel TCP does retransmission smart,
userspace UDP apps do it stupidly, so let's turn off the feature
instead of fixing userspace.
Right?
Sorry, fix this correctly in the user apps. Putting the
blame on UDP path MTU discovery is placing it in the
wrong spot.
^ permalink raw reply
* Re: UDP path MTU discovery
From: David Miller @ 2010-03-26 3:26 UTC (permalink / raw)
To: rick.jones2; +Cc: gdt, netdev
In-Reply-To: <4BAC0577.7070803@hp.com>
From: Rick Jones <rick.jones2@hp.com>
Date: Thu, 25 Mar 2010 17:53:11 -0700
> So, presuming it is indeed a bug what form might a fix take? Are you
> suggesting there should be a way for an application to say "Please let
> me see/know about the ICMP messages?" Is that option available on
> other platforms as a platform-specific extension?
We already provide this information.
The socket ends up with EMSGSIZE in it's error queue, so the next time
the application does I/O it sees that error immediately from the
read/write call and thus knows that path MTU arrived.
^ permalink raw reply
* Re: [Regression] r8169: enable 64-bit DMA by default for PCI Express devices (v2)
From: David Miller @ 2010-03-26 3:29 UTC (permalink / raw)
To: hancockrwd; +Cc: joern, torvalds, linux-kernel, netdev, romieu
In-Reply-To: <51f3faa71003251756h17374375yd3a5d2acee2ffab9@mail.gmail.com>
From: Robert Hancock <hancockrwd@gmail.com>
Date: Thu, 25 Mar 2010 18:56:03 -0600
> Francois, ping? Is there anyone else that has access to this kind of
> information about these chips?
Francois didn't have time to resolve a remotely exploitable
DoS bug in this driver, I doubt he has time to resolve something
like this.
It isn't going to be resolved any time soon, so I'm reverting
the guilty change now.
Thanks.
^ permalink raw reply
* Re: [PATCH RFC] fix problems with NETIF_F_HIGHDMA in networking drivers v2
From: FUJITA Tomonori @ 2010-03-26 3:33 UTC (permalink / raw)
To: hancockrwd
Cc: fujita.tomonori, linux-kernel, netdev, linux-usb, davem, bzolnier
In-Reply-To: <51f3faa71003251803q7ccec5d5x82bc277c590e2848@mail.gmail.com>
On Thu, 25 Mar 2010 19:03:37 -0600
Robert Hancock <hancockrwd@gmail.com> wrote:
> This seems like it could be a reasonable approach. The only thing is
> that in this code you're returning 1 if the parent device has no DMA
> mask set. Wouldn't it make more sense to return 0 in this case? I'm
> assuming that in that situation it's a virtual device not backed by
> any hardware and there should be no DMA mask restriction...
I chose the safer option because I don't know enough how net_device
structure is used. If returning zero in such case is always safe, it's
fine by me. any example of such virtual device driver?
^ permalink raw reply
* Re: [PATCH RFC] fix problems with NETIF_F_HIGHDMA in networking drivers v2
From: David Miller @ 2010-03-26 3:35 UTC (permalink / raw)
To: fujita.tomonori; +Cc: hancockrwd, linux-kernel, netdev, linux-usb, bzolnier
In-Reply-To: <20100326123250A.fujita.tomonori@lab.ntt.co.jp>
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Date: Fri, 26 Mar 2010 12:33:12 +0900
> On Thu, 25 Mar 2010 19:03:37 -0600
> Robert Hancock <hancockrwd@gmail.com> wrote:
>
>> This seems like it could be a reasonable approach. The only thing is
>> that in this code you're returning 1 if the parent device has no DMA
>> mask set. Wouldn't it make more sense to return 0 in this case? I'm
>> assuming that in that situation it's a virtual device not backed by
>> any hardware and there should be no DMA mask restriction...
>
> I chose the safer option because I don't know enough how net_device
> structure is used. If returning zero in such case is always safe, it's
> fine by me. any example of such virtual device driver?
Like Fujita I'd rather play it safe here.
Even for virtual devices, DMA information up to the root bus
ought to be sane.
^ permalink raw reply
* [PATCH] ipv6: Preserve pervious behavior in ipv6_link_dev_addr().
From: David Miller @ 2010-03-26 4:26 UTC (permalink / raw)
To: netdev; +Cc: shemminger
I just committed the following to net-next-2.6
ipv6: Preserve pervious behavior in ipv6_link_dev_addr().
Use list_add_tail() to get the behavior we had before
the list_head conversion for ipv6 address lists.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/ipv6/addrconf.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 68e5809..9995683 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -578,7 +578,7 @@ ipv6_link_dev_addr(struct inet6_dev *idev, struct inet6_ifaddr *ifp)
break;
}
- list_add(&ifp->if_list, p);
+ list_add_tail(&ifp->if_list, p);
}
static u32 ipv6_addr_hash(const struct in6_addr *addr)
--
1.7.0.3
^ permalink raw reply related
* Re: mmotm 2010-03-23 - IPv6 warnings...
From: David Miller @ 2010-03-26 4:40 UTC (permalink / raw)
To: Valdis.Kletnieks; +Cc: akpm, netdev, linux-kernel, shemminger
In-Reply-To: <4788.1269535530@localhost>
From: Valdis.Kletnieks@vt.edu
Date: Thu, 25 Mar 2010 12:45:30 -0400
> On Wed, 24 Mar 2010 18:42:26 EDT, Andrew Morton said:
>> On Wed, 24 Mar 2010 21:36:41 -0400 Valdis.Kletnieks@vt.edu wrote:
>>
>> > On Tue, 23 Mar 2010 15:34:59 PDT, akpm@linux-foundation.org said:
>> > > The mm-of-the-moment snapshot 2010-03-23-15-34 has been uploaded to
>> > >
>> > > http://userweb.kernel.org/~akpm/mmotm/
>> >
>> > Seen in my dmesg. It may be relevant that I'm at home, and my IPv6
>> > prefix arrives via a PPP VPN connection. This happened about 20-25 seconds
>> > after I launched pppd.
>>
>> Yes, thanks, I get the same - it doesn't seem to break anything. It
>> also happens some time after boot has completed.
>
> Just doing an 'ifup eth0' on a network with IPv6 on it is sufficient.
I just checked the following into net-next-2.6 which should at least
kill that dmesg log.
If you still are missing your ipv6 addresses, let me know how
they normally get added, maybe that's enough to let me figure
it out.
Thanks.
ipv6: Fix result generation in ipv6_get_ifaddr().
Finishing naturally from hlist_for_each_entry(x, ...) does not result
in 'x' being NULL.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/ipv6/addrconf.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 9995683..21b4c9e 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1341,9 +1341,9 @@ EXPORT_SYMBOL(ipv6_chk_prefix);
struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *addr,
struct net_device *dev, int strict)
{
- struct inet6_ifaddr *ifp = NULL;
- struct hlist_node *node;
+ struct inet6_ifaddr *ifp, *result = NULL;
unsigned int hash = ipv6_addr_hash(addr);
+ struct hlist_node *node;
rcu_read_lock_bh();
hlist_for_each_entry_rcu(ifp, node, &inet6_addr_lst[hash], addr_lst) {
@@ -1352,6 +1352,7 @@ struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *add
if (ipv6_addr_equal(&ifp->addr, addr)) {
if (dev == NULL || ifp->idev->dev == dev ||
!(ifp->scope&(IFA_LINK|IFA_HOST) || strict)) {
+ result = ifp;
in6_ifa_hold(ifp);
break;
}
@@ -1359,7 +1360,7 @@ struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *add
}
rcu_read_unlock_bh();
- return ifp;
+ return result;
}
/* Gets referenced address, destroys ifaddr */
--
1.7.0.3
^ permalink raw reply related
* IPv6 neighbor advertisements
From: Haile Seifu @ 2010-03-26 5:51 UTC (permalink / raw)
To: netdev
Hi,
I have been running some tests involving IPv6 neighbor discovery.
When a neighbor advertisement is received by the Kernel and the
follwing is true:
- the advertisement's solicited flag is set,
- the advertisement's override flag is clear,
- the advertisement's link layer address differs from what is in
the corresponding neighbor cache entry, and
- the corresponding neighbor cache entry state is neither
INCOMPLETE or REACHABLE
then it appears that the cache entry gets updated from STALE/DELAY to
REACHABLE. According to RFC 2461/4861 section
7.2.5, the advertisement in the above scenario should be ignored and
MUST NOT update the neighbor cache entry
I was curious about why the following code in net/core/neighbour.c,
function neigh_update, updates the neigh->confirmed
and neigh->updated fields (in lines 1039-1041) when the else-condition
(the scenario described above) is still possible in
lines 1055-1056.
By the way, the "new" flag is set based on the solicited flag of the
neighbor advertisement, so the if-condition on line 1039 will
be true.
net/core/neighbour.c (version linux-2.6.34-rc1):
982 int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
983 u32 flags)
984 {
...
...
...
1039 if (new & NUD_CONNECTED)
1040 neigh->confirmed = jiffies;
1041 neigh->updated = jiffies;
1042
1043 /* If entry was valid and address is not changed,
1044 do not change entry state, if new one is STALE.
1045 */
1046 err = 0;
1047 update_isrouter = flags & NEIGH_UPDATE_F_OVERRIDE_ISROUTER;
1048 if (old & NUD_VALID) {
1049 if (lladdr != neigh->ha && !(flags &
NEIGH_UPDATE_F_OVERRIDE)) {
1050 update_isrouter = 0;
1051 if ((flags & NEIGH_UPDATE_F_WEAK_OVERRIDE) &&
1052 (old & NUD_CONNECTED)) {
1053 lladdr = neigh->ha;
1054 new = NUD_STALE;
1055 } else
1056 goto out;
1057 } else {
1058 if (lladdr == neigh->ha && new == NUD_STALE &&
1059 ((flags & NEIGH_UPDATE_F_WEAK_OVERRIDE) ||
1060 (old & NUD_CONNECTED))
1061 )
1062 new = old;
1063 }
1064 }
.
.
.
^ permalink raw reply
* Re: IPv6 neighbor advertisements
From: David Miller @ 2010-03-26 6:07 UTC (permalink / raw)
To: haile.seifu; +Cc: netdev
In-Reply-To: <aa711df21003252251x524d7dd2hac4a88103d0ae3a@mail.gmail.com>
Keep in mind that net/core/neighbour.c is for IPV4 ARP too.
So if there is a conflict between what is allowed by ipv4
and what is allowed by ipv6 in this scenerio, that would
explain it.
^ permalink raw reply
* [PATCH] benet: Fix compile warnnings in drivers/net/benet/be_ethtool.c
From: wzt.wzt @ 2010-03-26 6:12 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, linux-drivers, sathyap
Fix the following warnings:
be_ethtool.c:493: warning: integer constant is too large for 'long' type
be_ethtool.c:493: warning: integer constant is too large for 'long' type
Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>
---
drivers/net/benet/be_ethtool.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/benet/be_ethtool.c b/drivers/net/benet/be_ethtool.c
index 9560d48..51e1065 100644
--- a/drivers/net/benet/be_ethtool.c
+++ b/drivers/net/benet/be_ethtool.c
@@ -490,7 +490,7 @@ be_test_ddr_dma(struct be_adapter *adapter)
{
int ret, i;
struct be_dma_mem ddrdma_cmd;
- u64 pattern[2] = {0x5a5a5a5a5a5a5a5a, 0xa5a5a5a5a5a5a5a5};
+ u64 pattern[2] = {0x5a5a5a5a5a5a5a5aULL, 0xa5a5a5a5a5a5a5a5ULL};
ddrdma_cmd.size = sizeof(struct be_cmd_req_ddrdma_test);
ddrdma_cmd.va = pci_alloc_consistent(adapter->pdev, ddrdma_cmd.size,
--
1.6.5.3
^ permalink raw reply related
* Re: [PATCH RFC 1/2] flow: virtualize get and entry deletion methods
From: Timo Teräs @ 2010-03-26 6:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev, herbert
In-Reply-To: <20100325.122611.267401605.davem@davemloft.net>
David Miller wrote:
> From: Timo Teras <timo.teras@iki.fi>
> Date: Thu, 25 Mar 2010 11:24:50 +0200
>
>> This allows to validate the cached object before returning it.
>> It also allows to destruct object properly, if the last reference
>> was held in flow cache. This is also a prepartion for caching
>> bundles in the flow cache.
>>
>> In return for virtualizing the methods, we save on:
>> - not having to regenerate the whole flow cache on policy removal:
>> each flow matching a killed policy gets refreshed as the getter
>> function notices it smartly.
>> - we do not have to call flow_cache_flush from policy gc, since the
>> flow cache now properly deletes the object if it had any references
>>
>> This also means the flow cache entry deletion does more work. If
>> it's too slow now, may have to implement delayed deletion of flow
>> cache entries. But this is a save because this enables immediate
>> deletion of policies and bundles.
>>
>> Signed-off-by: Timo Teras <timo.teras@iki.fi>
>
> I'm concerned about the new costs being added here.
>
> We have to now take the policy lock as a reader every time the flow
> cache wants to grab a reference. So we now have this plus the
> indirect function call new overhead.
If we want to have the flow cache generic, we pretty much need
indirect calls. But considering that it might make sense to cache
bundles, or "xfrm cache entries" on all flow directions (so we can
track both the main and sub policies) we could make it specialized.
> Maybe we can make the dead state check safe to do asynchronously
> somehow? I wonder if the policy layer is overdue for an RCU
> conversion or similar.
I looked at the code and the policy lock is not needed much anymore.
I think it was most heavily used to protected ->bundles which is
now removed. But yes, I also previously said that ->walk.dead should
probably be converted to atomic_t. It is only written once when
the policy is killed. So we can make it accessible without the lock.
Considering that the whole cache was broken previously, and we
needed to take write lock on policy for each forwarded packet,
it does not sound that bad. Apparently locally originating traffic
directly to xfrm destination (not via gre) would get cached on the
socket dst cache and avoids the xfrm_lookup on fast path entirely(?).
We can get away from the per-cpu design with RCU hash. But I think
we still need to track the hash entries similar to this. Though,
there's probably some other tricks doable with RCU which I'm not
all familiar with. I will take a quick look on the rcu thingy
Herbert mentioned earlier.
> Anyways, something to think about. Otherwise I don't mind these
> changes.
Ok, I'll add "convert walk.dead to atomic_t" so we can access it
without a lock.
I did also notice that the policy locking is not right exactly.
E.g. migration can touch templates, and we read templates currently
without locks. So I think bundle creation should be protected
with policy read lock. But even this can probably be avoided by
RCU type bundle creation. We just take bundle genid before starting
to create it, create bundle, and check if genid was changed while
doing this we retry.
We might even get away from policy->lock all together. In most
places it's only used to protect walk.dead. And bundle creation
can be synchronizes as above. The only remaining place seems to
be the timer function. I think it's safe to remove locking there
too, and synchronize using timer deletion. All this is because
any changes to policy will result in xfrm_policy replacement:
the old is killed and new one inserted atomically.
Do you think this would work?
- Timo
^ 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