* RE: [PATCH] Export smbios strings associated with onboard devicesto sysfs
From: Narendra_K @ 2010-03-02 17:42 UTC (permalink / raw)
To: achiang
Cc: netdev, linux-hotplug, linux-pci, Matt_Domsch, Jordan_Hargrave,
Sandeep_K_Shandilya, Charles_Rose, Shyam_Iyer
In-Reply-To: <20100225214020.GA15010@ldl.fc.hp.com>
> -----Original Message-----
> From: Alex Chiang [mailto:achiang@hp.com]
> Sent: Friday, February 26, 2010 3:10 AM
> To: K, Narendra
> Cc: netdev@vger.kernel.org; linux-hotplug@vger.kernel.org; linux-
> pci@vger.kernel.org; Domsch, Matt; Hargrave, Jordan; Shandilya,
Sandeep
> K; Rose, Charles; Iyer, Shyam
> Subject: Re: [PATCH] Export smbios strings associated with onboard
> devicesto sysfs
>
> * Narendra K <Narendra_K@dell.com>:
> >
> > * We have been having discussions in the netdev list about
> > creating multiple names for the network interfaces to bring
> > determinism into the way network interfaces are named in the
> > OSes. In specific, "eth0 in the OS does not always map to the
> > integrated NIC Gb1 as labelled on the chassis".
>
> Yes, I agree that this is a real problem that we do not handle
> well today.
>
> > 1.Export smbios strings of onboard devices, to sysfs. For example -
> >
> > cat /sys/class/net/eth0/device/smbiosname
> > Embedded NIC 2
> >
> > cat /sys/bus/pci/devices/0000\:03\:00.0/smbiosname
> > Embedded NIC 2
>
> I agree with this concept, but I don't like the interface.
>
> The name "smbiosname" isn't the proper level of abstraction. We
> don't want users to care what firmware standard is providing the
> name (think smbios vs acpi vs open firmware...).
>
> We learned this lesson with exposing ACPI interfaces. Let's not
> make the same mistake here.
>
> Something like "firmwarename", "fwname", "platformname" etc. is
> generic, and then the interface will make sense for platforms
> that do not implement SMBIOS.
>
> I don't particularly care which name you choose as long as it's
> properly generic.
>
> As far as implementation goes, I've been thinking about this
> problem for a while now, and I keep wanting to use the
> pci_create_slot() API, but am still a little on the fence about
> it.
>
> The pros:
> - all you have to do is write a simple little driver that
> can read SMBIOS to get PCI bus:devfn and the name, and
> then you call pci_create_slot(). Then you get all sorts
> of stuff for free, like sysfs exposure, proper
> refcounting (important given that the PCI logical
> hotplug interface (/sys/bus/pci/rescan and friends) can
> be used to remove onboard devices), etc.
>
> - see drivers/acpi/pci_slot.c for an example of how to
> detect slots and then register them.
>
> The cons:
> - the user interface is /sys/bus/pci/slots/<name>
>
> I don't know if that is an appropriate interface, since
> technically an onboard device isn't in a slot. But maybe
> if you did something like:
>
> /sys/bus/pci/slots/onboard0
> /sys/bus/pci/slots/onboard1
>
> that might make sense.
>
> Or...
> /sys/bus/pci/onboard/<name>
>
> I read through the patch, but given that the implementation
> strategy might change based on my comments, will hold off and see
> how the conversation develops.
Thanks. I will wait too, to see how the discussion develops on this
method of implementation.
With regards,
Narendra K
^ permalink raw reply
* Re: [PATCH 1/1] bnx2x: Tx barriers and locks
From: Michael Chan @ 2010-03-02 17:26 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Vladislav Zolotarov, netdev@vger.kernel.org, davem@davemloft.net,
Eilon Greenstein, Matthew Carlson
In-Reply-To: <20100302165946.GC6491@dhcp-lab-161.englab.brq.redhat.com>
On Tue, 2010-03-02 at 08:59 -0800, Stanislaw Gruszka wrote:
> On Tue, Mar 02, 2010 at 08:18:44AM -0800, Michael Chan wrote:
> > Stanislaw Gruszka wrote:
> >
> > > On Tue, Mar 02, 2010 at 04:50:59AM -0800, Vladislav Zolotarov wrote:
> > > > Stanislaw barrier() is not a memory barrier - it's a
> > > compiler barrier. I don't think removing it from
> > > bnx2x_tx_avail() will improve anything. If u think I'm wrong,
> > > could u, pls., provide a specific example.
> > >
> > > Only improvement is removing confusing code, And comment like
> > > "Tell compiler that prod and cons can change" is even more
> > > confusing. If you think I'm wrong, just tell as why that
> > > barrier is needed :)
> >
> > The barrier (compiler barrier at least) is required in
> > bnx2x_tx_avail(). The status block index can be updated by DMA and
> > the compiler doesn't know it (because it is considered wrong to
>
> If you are telling status block index you mean which variable ?
The fp-> fields which can be updated by NAPI poll based on new status
block DMA.
>
> > declare the status block as volatile). Near the end of
> > bnx2x_start_xmit() where we call bnx2x_tx_avail() twice. It is
> > possible that the compiler will optimize it and not look at the
> > status block in memory the second time.
>
> Ok, I'm trying to understand.
> --
^ permalink raw reply
* Re: [PATCH] Export smbios strings associated with onboard devices to sysfs
From: Narendra K @ 2010-03-02 17:33 UTC (permalink / raw)
To: matt_domsch
Cc: netdev, linux-hotplug, linux-pci, jordan_hargrave,
sandeep_k_shandilya, charles_rose, shyam_iyer
In-Reply-To: <EDA0A4495861324DA2618B4C45DCB3EE6122A2@blrx3m08.blr.amer.dell.com>
> > -----Original Message-----
> > From: Domsch, Matt [mailto:Matt_Domsch@Dell.com]
> > Sent: Friday, February 26, 2010 2:19 AM
> > To: K, Narendra
> > Cc: netdev@vger.kernel.org; linux-hotplug@vger.kernel.org; linux-
> > pci@vger.kernel.org; Hargrave, Jordan; Shandilya, Sandeep K; Rose,
> > Charles; Iyer, Shyam
> > Subject: Re: [PATCH] Export smbios strings associated with onboard
> > devices to sysfs
> >
> > On Thu, Feb 25, 2010 at 02:29:42PM -0600, K, Narendra wrote:
> > > 1.Export smbios strings of onboard devices, to sysfs. For example -
> >
> > I like the idea in this patch, which exports this additional
> > smbios-provided string in sysfs. This removes the need to parse the
> > SMBIOS table in userspace, which requires root privs.
> >
> > The concept is also extensible to other methods (e.g. ACPI) that I
> > expect will appear for the platform BIOS to provide naming hints to
> > the OS - I want those to appear in sysfs also.
> >
> > now, for the patch:
> >
> > > + if (!(strcmp(attr_name(bus->dev_attrs[i]),
> "smbiosname")))
> > {
> > > + if (!smbiosname_string_is_valid(dev, NULL))
> > > + continue;
> > > + }
> > > error = device_create_file(dev, &bus->dev_attrs[i]);
> > > if (error) {
> > > while (--i >= 0)
> >
> > This cannot go in bus.c. It needs to go in pci-sysfs.c.
> >
Resending the patch with review comments incorporated.
Signed-off-by: Jordan Hargrave <Jordan_Hargrave@dell.com>
Signed-off-by: Narendra K <Narendra_K@dell.com>
---
drivers/firmware/dmi_scan.c | 23 ++++++++++++++++++
drivers/pci/pci-sysfs.c | 54 +++++++++++++++++++++++++++++++++++++++++++
include/linux/dmi.h | 9 +++++++
3 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 31b983d..1d10663 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -278,6 +278,28 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
list_add_tail(&dev->list, &dmi_devices);
}
+static void __init dmi_save_devslot(int id, int seg, int bus, int devfn, const char *name)
+{
+ struct dmi_devslot *slot;
+
+ slot = dmi_alloc(sizeof(*slot) + strlen(name) + 1);
+ if (!slot) {
+ printk(KERN_ERR "dmi_save_devslot: out of memory.\n");
+ return;
+ }
+ slot->id = id;
+ slot->seg = seg;
+ slot->bus = bus;
+ slot->devfn = devfn;
+
+ strcpy((char *)&slot[1], name);
+ slot->dev.type = DMI_DEV_TYPE_DEVSLOT;
+ slot->dev.name = &slot[1];
+ slot->dev.device_data = slot;
+
+ list_add(&slot->dev.list, &dmi_devices);
+}
+
static void __init dmi_save_extended_devices(const struct dmi_header *dm)
{
const u8 *d = (u8*) dm + 5;
@@ -286,6 +308,7 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
if ((*d & 0x80) == 0)
return;
+ dmi_save_devslot(-1, *(u16 *)(d+2), *(d+4), *(d+5), dmi_string_nosave(dm, *(d-1)));
dmi_save_one_device(*d & 0x7f, dmi_string_nosave(dm, *(d - 1)));
}
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 807224e..85d5d79 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -142,6 +142,54 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
(u8)(pci_dev->class));
}
+#ifdef CONFIG_DMI
+#include <linux/dmi.h>
+static ssize_t
+smbiosname_string_is_valid(struct device *dev, char *buf)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ struct dmi_device *dmi;
+ struct dmi_devslot *dslot;
+ int bus;
+ int devfn;
+
+ bus = pdev->bus->number;
+ devfn = pdev->devfn;
+
+ dmi = NULL;
+ while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEVSLOT, NULL, dmi)) != NULL) {
+ dslot = dmi->device_data;
+ if (dslot && dslot->bus == bus && dslot->devfn == devfn) {
+ if (buf)
+ return scnprintf(buf, PAGE_SIZE, "%s\n", dmi->name);
+ return strlen(dmi->name);
+ }
+ }
+}
+
+static ssize_t
+smbiosname_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ return smbiosname_string_is_valid(dev, buf);
+
+}
+
+struct smbios_attribute {
+ struct attribute attr;
+ ssize_t(*show) (struct device * edev, char *buf);
+ int (*test) (struct device * edev, char *buf);
+};
+
+#define SMBIOSNAME_DEVICE_ATTR(_name, _mode, _show, _test) \
+struct smbios_attribute smbios_attr_##_name = { \
+ .attr = {.name = __stringify(_name), .mode = _mode }, \
+ .show = _show, \
+ .test = _test, \
+};
+
+static SMBIOSNAME_DEVICE_ATTR(smbiosname, 0444, smbiosname_show, smbiosname_string_is_valid);
+#endif
+
static ssize_t is_enabled_store(struct device *dev,
struct device_attribute *attr, const char *buf,
size_t count)
@@ -1140,6 +1188,7 @@ static int __init pci_sysfs_init(void)
{
struct pci_dev *pdev = NULL;
int retval;
+ struct smbios_attribute *attr = &smbios_attr_smbiosname;
sysfs_initialized = 1;
for_each_pci_dev(pdev) {
@@ -1148,6 +1197,11 @@ static int __init pci_sysfs_init(void)
pci_dev_put(pdev);
return retval;
}
+#ifdef CONFIG_DMI
+ /* if the device does not have an associated smbios string in the smbios table, do not create this attribute */
+ if (attr->test && attr->test(&pdev->dev, NULL))
+ sysfs_create_file(&pdev->dev.kobj, &attr->attr);
+#endif
}
return 0;
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index a8a3e1a..cc57c3a 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -20,6 +20,7 @@ enum dmi_device_type {
DMI_DEV_TYPE_SAS,
DMI_DEV_TYPE_IPMI = -1,
DMI_DEV_TYPE_OEM_STRING = -2,
+ DMI_DEV_TYPE_DEVSLOT = -3,
};
struct dmi_header {
@@ -37,6 +38,14 @@ struct dmi_device {
#ifdef CONFIG_DMI
+struct dmi_devslot {
+ struct dmi_device dev;
+ int id;
+ int seg;
+ int bus;
+ int devfn;
+};
+
extern int dmi_check_system(const struct dmi_system_id *list);
const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
extern const char * dmi_get_system_info(int field);
--
1.6.5.2
With regards,
Narendra K
^ permalink raw reply related
* Re: [Bridge] [PATCH] bridge: per-cpu packet statistics
From: Stephen Hemminger @ 2010-03-02 17:23 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, bridge, David Miller
In-Reply-To: <1267520579.2964.27.camel@edumazet-laptop>
On Tue, 02 Mar 2010 10:02:59 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
>
> Le lundi 01 mars 2010 à 16:16 -0800, Stephen Hemminger a écrit :
>
> > + for_each_online_cpu(cpu) {
> > + const struct br_cpu_netstats *bstats
> > + = per_cpu_ptr(br->stats, cpu);
> > +
> > + stats->rx_bytes += bstats->rx_bytes;
> > + stats->rx_packets += bstats->rx_packets;
> > + }
>
> And last point, we should use for_each_possible_cpu() here
>
> Here is your patch with all my comments integrated :
>
> 1) Use txq->{tx_bytes|tx_packets} counter
>
> 2) alloc_percpu(struct ...) instead of alloc_percpu(sizeof(struct ...))
>
> 3) free_netdev() in destructor
>
> 4) for_each_possible_cpu() instead of for_each_online_cpu()
>
> 5) br_get_stats() use local variables for the sake of concurrent users
No need, bridge has no queue!
^ permalink raw reply
* Re: [Bridge] [PATCH] bridge: per-cpu packet statistics
From: Stephen Hemminger @ 2010-03-02 17:22 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, bridge, David Miller
In-Reply-To: <1267509690.2843.5.camel@edumazet-laptop>
On Tue, 02 Mar 2010 07:01:30 +0100
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 01 mars 2010 à 16:16 -0800, Stephen Hemminger a écrit :
> > The shared packet statistics are a potential source of slow down
> > on bridged traffic. Convert to per-cpu array, but only keep those
> > statistics which change per-packet.
> >
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >
> > ---
> >
> > net/bridge/br_device.c | 43 ++++++++++++++++++++++++++++++++++++++-----
> > net/bridge/br_if.c | 6 ++++++
> > net/bridge/br_input.c | 5 +++--
> > net/bridge/br_private.h | 8 ++++++++
> > 4 files changed, 55 insertions(+), 7 deletions(-)
> >
> > --- a/net/bridge/br_device.c 2010-03-01 08:22:23.476657998 -0800
> > +++ b/net/bridge/br_device.c 2010-03-01 15:31:36.737227465 -0800
> > @@ -26,11 +26,12 @@ netdev_tx_t br_dev_xmit(struct sk_buff *
> > const unsigned char *dest = skb->data;
> > struct net_bridge_fdb_entry *dst;
> > struct net_bridge_mdb_entry *mdst;
> > + struct br_cpu_netstats *brstats = this_cpu_ptr(br->stats);
> >
> > - BR_INPUT_SKB_CB(skb)->brdev = dev;
> > + brstats->tx_packets++;
> > + brstats->tx_bytes += skb->len;
>
>
> On TX path, this is not really necessary, since we already dirtied
> txq->lock before calling br_dev_xmit(), we can use txq->tx_packets and
> txq->tx_bytes for free ?
Bridge is already using lockless transmit LLTX, so tx_lock is not touched.
^ permalink raw reply
* Re: [PATCH 1/1] bnx2x: Tx barriers and locks
From: Stanislaw Gruszka @ 2010-03-02 16:59 UTC (permalink / raw)
To: Michael Chan
Cc: Vladislav Zolotarov, netdev@vger.kernel.org, davem@davemloft.net,
Eilon Greenstein, Matthew Carlson
In-Reply-To: <C27F8246C663564A84BB7AB34397724218312F8FD4@IRVEXCHCCR01.corp.ad.broadcom.com>
On Tue, Mar 02, 2010 at 08:18:44AM -0800, Michael Chan wrote:
> Stanislaw Gruszka wrote:
>
> > On Tue, Mar 02, 2010 at 04:50:59AM -0800, Vladislav Zolotarov wrote:
> > > Stanislaw barrier() is not a memory barrier - it's a
> > compiler barrier. I don't think removing it from
> > bnx2x_tx_avail() will improve anything. If u think I'm wrong,
> > could u, pls., provide a specific example.
> >
> > Only improvement is removing confusing code, And comment like
> > "Tell compiler that prod and cons can change" is even more
> > confusing. If you think I'm wrong, just tell as why that
> > barrier is needed :)
>
> The barrier (compiler barrier at least) is required in
> bnx2x_tx_avail(). The status block index can be updated by DMA and
> the compiler doesn't know it (because it is considered wrong to
If you are telling status block index you mean which variable ?
> declare the status block as volatile). Near the end of
> bnx2x_start_xmit() where we call bnx2x_tx_avail() twice. It is
> possible that the compiler will optimize it and not look at the
> status block in memory the second time.
Ok, I'm trying to understand.
^ permalink raw reply
* Re: Yahoo/Msn Ticket Number 3XTA932CL9
From: Yahoo/Msn© @ 2010-03-02 16:35 UTC (permalink / raw)
To: info
£1,000,000.00 has been awarded to you in the Yahoo/Msn Grant Award,send us your Names,Tel,Country:
^ permalink raw reply
* Re: Yahoo/Msn Ticket Number 3XTA932CL9
From: Yahoo/Msn© @ 2010-03-02 16:26 UTC (permalink / raw)
To: info
£1,000,000.00 has been awarded to you in the Yahoo/Msn Grant Award,send us your Names,Tel,Country:
^ permalink raw reply
* RE: [PATCH 1/1] bnx2x: Tx barriers and locks
From: Vladislav Zolotarov @ 2010-03-02 16:21 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Michael Chan, netdev@vger.kernel.org, davem@davemloft.net,
Eilon Greenstein, Matthew Carlson
In-Reply-To: <20100302135555.GC2362@dhcp-lab-161.englab.brq.redhat.com>
> -----Original Message-----
> From: Stanislaw Gruszka [mailto:sgruszka@redhat.com]
> Sent: Tuesday, March 02, 2010 3:56 PM
> To: Vladislav Zolotarov
> Cc: Michael Chan; netdev@vger.kernel.org;
> davem@davemloft.net; Eilon Greenstein; Matthew Carlson
> Subject: Re: [PATCH 1/1] bnx2x: Tx barriers and locks
>
> On Tue, Mar 02, 2010 at 04:50:59AM -0800, Vladislav Zolotarov wrote:
> > Stanislaw barrier() is not a memory barrier - it's a
> compiler barrier. I don't think removing it from
> bnx2x_tx_avail() will improve anything. If u think I'm wrong,
> could u, pls., provide a specific example.
>
> Only improvement is removing confusing code, And comment like
> "Tell compiler that prod and cons can change" is even more
> confusing. If you think I'm wrong, just tell as why that
> barrier is needed :)
Both the coment and the barrier() ensures that compiler doesn't optimize the reading of the consumer and producers outside the inline function. This is what was meant when the function has been written so I don't think that either the remark or a barrier() itself are confusing.
>
> For bnx2 and tg3 that would be removal of smp_mb().
>
> > Let's recall that the bxn2x_start_xmit() is called under
> tx_lock(), so no additional memory barriers needed to ensure
> that bnx2x_tx_avail() has fresh values of BD consumer and
> producer at the beginning of bnx2x_start_xmit().
>
> Memory barriers are not about refreshing anything, they are
> about ordering.
>
> > That's why we don't need the more complicated logic putting
> the queue asleep at the beginning of bnx2x_start_xmit() as we
> need at the end. That's why I think your "goto" change in the
> beginning of bnx2x_start_xmit() is suboptimal.
>
> You may have right, about that smb_mb() is not needed at the beginning
> of start_xmit(), but I'm not sure for that, since we are
> changing tx_bd_cons
> without netif_tx_lock and setting XOFF bit there. If we can
> assume that
> at the beginning of start_xmit() there is always enough space
> it hw queue,
> we can just remove tx_avail() check. Otherwise IMHO is better
> to put queue
> asleep in safe way.
The driver (bnx2x, Michael and Matthuew may elaborate on bnx2 and tg3 better) is written the way that is meant to ensure that there is always place on the ring at the beginning of bnx2x_start_xmit(). The check at the beginning ensures that nothing is broken. And if it's broken - it's a bug that should be fixed and we may not remove that check because if we do we will not even know that there was something wrong. But as long as there is nothing wrong - the current implementation of the tx_avail() check at the beginning of bnx2x_start_xmit() is absolutely safe as it is now.
>
> About performance, adding smp_mb() on bug/unused code path
> does not hurt
> much :)
It doesn't help either.... ;)
>
> Cheers
> Stanislaw
>
> > > -----Original Message-----
> > > From: Stanislaw Gruszka [mailto:sgruszka@redhat.com]
> > > Sent: Tuesday, March 02, 2010 1:31 PM
> > > To: Michael Chan
> > > Cc: Vladislav Zolotarov; netdev@vger.kernel.org;
> > > davem@davemloft.net; Eilon Greenstein; Matthew Carlson
> > > Subject: Re: [PATCH 1/1] bnx2x: Tx barriers and locks
> > >
> > > On Mon, Mar 01, 2010 at 09:59:07AM -0800, Michael Chan wrote:
> > > > > There is still difference between what we have in bnx2x
> > > and bnx2/tg3
> > > > > regarding memory barriers in tx_poll/start_xmit code.
> > > Mainly we have
> > > > > smp_mb() in bnx2/tg3_tx_avail(), and in bnx2/tg3_tx_int()
> > > is smp_mb()
> > > > > not smp_wmb(). I do not see that bnx2x is wrong, but
> > > would like to know
> > > > > why there is a difference, maybe bnx2/tg3 should be changed?
> > > > >
> > > >
> > > > The memory barrier in tx_int() is to make the tx index
> update happen
> > > > before the netif_tx_queue_stopped() check. The barrier is
> > > to prevent a
> > > > situation like this:
> > > >
> > > > CPU0 CPU1
> > > > start_xmit()
> > > > if (tx_ring_full) {
> > > > tx_int()
> > > >
> > > if (!netif_tx_queue_stopped)
> > > > netif_tx_stop_queue()
> > > > if (!tx_ring_full)
> > > >
> > > update_tx_index
> > > > netif_tx_wake_queue()
> > > > }
> > > >
> > > >
> > > > The update_tx_index code is before the if statement in
> > > program order,
> > > > but the CPU and/or compiler can reorder it as shown above.
> > > smp_mb() will
> > > > prevent that. Will smp_wmb() prevent that as well?
> > >
> > > No. smp_wmb() affect only write orders on CPU1 performing
> tx_int(), so
> > > that should be fixed in bnx2x.
> > >
> > > Regarding memory barrier in tx_avail(), I don't think it its
> > > needed for
> > > anything, except maybe usage at the beginning of
> > > start_xmit(), but we can
> > > just remove that like in the patch below. I going to post
> "official"
> > > patches for tg3, bnx2 and bnx2x, if no nobody has nothing against.
> > >
> > > diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> > > index ed785a3..0f406b8 100644
> > > --- a/drivers/net/bnx2x_main.c
> > > +++ b/drivers/net/bnx2x_main.c
> > > @@ -893,7 +893,6 @@ static inline u16 bnx2x_tx_avail(struct
> > > bnx2x_fastpath *fp)
> > > u16 prod;
> > > u16 cons;
> > >
> > > - barrier(); /* Tell compiler that prod and cons can change */
> > > prod = fp->tx_bd_prod;
> > > cons = fp->tx_bd_cons;
> > >
> > > @@ -963,9 +962,8 @@ static int bnx2x_tx_int(struct
> bnx2x_fastpath *fp)
> > > * start_xmit() will miss it and cause the queue to be stopped
> > > * forever.
> > > */
> > > - smp_wmb();
> > > + smp_mb();
> > >
> > > - /* TBD need a thresh? */
> > > if (unlikely(netif_tx_queue_stopped(txq))) {
> > > /* Taking tx_lock() is needed to prevent
> > > reenabling the queue
> > > * while it's empty. This could have happen if
> > > rx_action() gets
> > > @@ -11177,10 +11175,9 @@ static netdev_tx_t
> > > bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > > struct eth_tx_bd *tx_data_bd, *total_pkt_bd = NULL;
> > > struct eth_tx_parse_bd *pbd = NULL;
> > > u16 pkt_prod, bd_prod;
> > > - int nbd, fp_index;
> > > + int nbd, fp_index, i, ret;
> > > dma_addr_t mapping;
> > > u32 xmit_type = bnx2x_xmit_type(bp, skb);
> > > - int i;
> > > u8 hlen = 0;
> > > __le16 pkt_size = 0;
> > >
> > > @@ -11195,10 +11192,9 @@ static netdev_tx_t
> > > bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > > fp = &bp->fp[fp_index];
> > >
> > > if (unlikely(bnx2x_tx_avail(fp) <
> > > (skb_shinfo(skb)->nr_frags + 3))) {
> > > - fp->eth_q_stats.driver_xoff++;
> > > - netif_tx_stop_queue(txq);
> > > BNX2X_ERR("BUG! Tx ring full when queue awake!\n");
> > > - return NETDEV_TX_BUSY;
> > > + ret = NETDEV_TX_BUSY;
> > >
> > > + goto stop_queue;
> > > }
> > >
> > > DP(NETIF_MSG_TX_QUEUED, "SKB: summed %x protocol %x
> > > protocol(%x,%x)"
> > > @@ -11426,19 +11422,24 @@ static netdev_tx_t
> > > bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > > mmiowb();
> > >
> > > fp->tx_bd_prod += nbd;
> > > -
> > > - if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) {
> > > - netif_tx_stop_queue(txq);
> > > - /* We want bnx2x_tx_int to "see" the updated tx_bd_prod
> > > - if we put Tx into XOFF state. */
> > > - smp_mb();
> > > - fp->eth_q_stats.driver_xoff++;
> > > - if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3)
> > > - netif_tx_wake_queue(txq);
> > > - }
> > > fp->tx_pkt++;
> > > +
> > > + ret = NETDEV_TX_OK;
> > > + if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3))
> > > + goto stop_queue;
> > > +
> > > + return ret;
> > >
> > > - return NETDEV_TX_OK;
> > > +stop_queue:
> > > + netif_tx_stop_queue(txq);
> > > + /* paired barrier is in bnx2x_tx_int(), update of tx_bd_cons
> > > + * have to be visable here, after we XOFF bit setting */
> > > + smp_mb();
> > > + fp->eth_q_stats.driver_xoff++;
> > > + if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3)
> > > + netif_tx_wake_queue(txq);
> > > +
> > > + return ret;
> > > }
> > >
> > > /* called with rtnl_lock */
> > >
> > >
>
>
^ permalink raw reply
* Re: [PATCH 1/1] bnx2x: Tx barriers and locks
From: Michael Chan @ 2010-03-02 16:18 UTC (permalink / raw)
To: 'Stanislaw Gruszka', Vladislav Zolotarov
Cc: netdev@vger.kernel.org, davem@davemloft.net, Eilon Greenstein,
Matthew Carlson
In-Reply-To: <20100302135555.GC2362@dhcp-lab-161.englab.brq.redhat.com>
Stanislaw Gruszka wrote:
> On Tue, Mar 02, 2010 at 04:50:59AM -0800, Vladislav Zolotarov wrote:
> > Stanislaw barrier() is not a memory barrier - it's a
> compiler barrier. I don't think removing it from
> bnx2x_tx_avail() will improve anything. If u think I'm wrong,
> could u, pls., provide a specific example.
>
> Only improvement is removing confusing code, And comment like
> "Tell compiler that prod and cons can change" is even more
> confusing. If you think I'm wrong, just tell as why that
> barrier is needed :)
The barrier (compiler barrier at least) is required in
bnx2x_tx_avail(). The status block index can be updated by DMA and
the compiler doesn't know it (because it is considered wrong to
declare the status block as volatile). Near the end of
bnx2x_start_xmit() where we call bnx2x_tx_avail() twice. It is
possible that the compiler will optimize it and not look at the
status block in memory the second time.
^ permalink raw reply
* Re: Notifier chains bug ?
From: Atis Elsts @ 2010-03-02 15:57 UTC (permalink / raw)
To: Oleg Kutkov; +Cc: netdev
In-Reply-To: <20100302153651.GC4014@hack>
I'm no expert, but IMHO for handling IP address changes you should use
function register_inetaddr_notifier() to register you callback and
then wait for NETDEV_UP and NETDEV_DOWN events.
On Tue, Mar 2, 2010 at 5:42 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Mar 02, 2010 at 07:50:01AM +0200, Oleg Kutkov wrote:
>>2010/3/2 Américo Wang <xiyou.wangcong@gmail.com>:
>>> On Tue, Mar 2, 2010 at 8:08 AM, Oleg Kutkov <elenbert@gmail.com> wrote:
>>>> Hello.
>>>> I try to used notifier chains for monitoring network devices events.
>>>> All works perfectly when just i'm connecting/disconnecting network cable or
>>>> up/down interface via ifconfig.
>>>> But when i try to change interface address - nothing happens. Notifier is
>>>> don't send any events :(
>>>
>>> I think you mean IP address? No, NETDEV_CHANGEADDR is for hardware
>>> address, not for IP address.
>>>
>>> If you were changing mac address, you will receive NETDEV_CHANGEADDR.
>>>
>>
>>Thank for quick answer.
>>Yes, i mean IP address. And what about NETDEV_CHANGE ?
>
> It is for rtnetlink state transition.
>
>>Is there possible for monitoring IP address/netmask changing ?
>>
>
> AFAIK, no.
>
> Cc'ing netdev experts...
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
~Atis
^ permalink raw reply
* Re: Notifier chains bug ?
From: Américo Wang @ 2010-03-02 15:42 UTC (permalink / raw)
To: Oleg Kutkov; +Cc: Américo Wang, linux-kernel, netdev
In-Reply-To: <5e9821061003012150y15e543cdkf6f23c3a7a47e4a3@mail.gmail.com>
On Tue, Mar 02, 2010 at 07:50:01AM +0200, Oleg Kutkov wrote:
>2010/3/2 Américo Wang <xiyou.wangcong@gmail.com>:
>> On Tue, Mar 2, 2010 at 8:08 AM, Oleg Kutkov <elenbert@gmail.com> wrote:
>>> Hello.
>>> I try to used notifier chains for monitoring network devices events.
>>> All works perfectly when just i'm connecting/disconnecting network cable or
>>> up/down interface via ifconfig.
>>> But when i try to change interface address - nothing happens. Notifier is
>>> don't send any events :(
>>
>> I think you mean IP address? No, NETDEV_CHANGEADDR is for hardware
>> address, not for IP address.
>>
>> If you were changing mac address, you will receive NETDEV_CHANGEADDR.
>>
>
>Thank for quick answer.
>Yes, i mean IP address. And what about NETDEV_CHANGE ?
It is for rtnetlink state transition.
>Is there possible for monitoring IP address/netmask changing ?
>
AFAIK, no.
Cc'ing netdev experts...
^ permalink raw reply
* Re: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Jan Engelhardt @ 2010-03-02 15:14 UTC (permalink / raw)
To: Pavel Emelyanov
Cc: Daniel Lezcano, Eric W. Biederman, hadi, Patrick McHardy,
Linux Netdev List, containers, Netfilter Development Mailinglist,
Ben Greear, Serge Hallyn, Matt Helsley
In-Reply-To: <4B8D28CF.8060304@parallels.com>
On Tuesday 2010-03-02 16:03, Pavel Emelyanov wrote:
>> I agree with all the points you and Pavel you talked about but I don't
>> feel comfortable to have the current process to switch the pid namespace
>> because of the process tree hierarchy (what will be the parent of the
>> process when you enter the pid namespace for example).
>
>The answer is - the one, that used to be. I see no problems with it.
>Do you?
But perhaps it could be named "namespacefd" instead of nsfd, to reduce
potential clashes (because glibc will usually just use the same name
when making the syscall available as a C function).
^ permalink raw reply
* Re: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Pavel Emelyanov @ 2010-03-02 15:03 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Eric W. Biederman, hadi, Patrick McHardy, Linux Netdev List,
containers, Netfilter Development Mailinglist, Ben Greear,
Serge Hallyn, Matt Helsley
In-Reply-To: <4B8AE8C1.1030305@free.fr>
> I agree with all the points you and Pavel you talked about but I don't
> feel comfortable to have the current process to switch the pid namespace
> because of the process tree hierarchy (what will be the parent of the
> process when you enter the pid namespace for example).
The answer is - the one, that used to be. I see no problems with it.
Do you?
^ permalink raw reply
* Re: [RFC PATCH]xfrm: fix perpetual bundles
From: jamal @ 2010-03-02 14:16 UTC (permalink / raw)
To: David Miller
Cc: steffen.klassert, herbert, kaber, yoshfuji, nakam, eric.dumazet,
netdev
In-Reply-To: <20100302.060606.73203093.davem@davemloft.net>
On Tue, 2010-03-02 at 06:06 -0800, David Miller wrote:
> Jamal please do some testing then I'll push Herbert's fix around.
I will get to it tonight.
cheers,
jamal
^ permalink raw reply
* Re: KS8695: possible NAPI issue
From: Dick Hollenbeck @ 2010-03-02 14:09 UTC (permalink / raw)
To: Yegor Yefremov; +Cc: netdev, figo1802, zealcook
In-Reply-To: <f69abfc31003020404y7af77fe5icefeee4644c7d08e@mail.gmail.com>
Yegor Yefremov wrote:
> I'm using 2.6.33 kernel and I noticed such a strange behavior:
>
> after system start I transfer one file via netcat from my development
> host, after this transfer the network is not functioning i.e. no pings
> possible etc.
>
> To narrow down the problem I checked out this commit
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=451f14439847db302e5104c44458b2dbb4b1829d.
> Till here the network driver is functioning as intended, but after
> NAPI introduction I have this issue. With latest git-pull of "Linus'
> kernel tree" I can't even ping right after the systems start.
>
> Any Ideas? What am I missing?
>
No idea, although I am using the same ARM chip, my kernel is at
2.6.30.5, and except for this occasional loss of connection I get, the
ethernet driver seems to work better than what you are reporting.
I would get your printk() working to the internal ring buffer, and then
simply instrument the key pieces of code with printk()s until the
situation becomes clear. You can also watch
# cat /proc/interrupts to see which interrupt handlers are firing,
before and after an activity. Reception of a frame all starts with an
interrupt handler usually.
printk() should be able to print even from within an interrupt handler,
so long as you are only going to that internal ring buffer.
Dick
> Regards,
> Yegor
>
>
^ permalink raw reply
* Re: [RFC PATCH]xfrm: fix perpetual bundles
From: Steffen Klassert @ 2010-03-02 14:06 UTC (permalink / raw)
To: jamal; +Cc: Herbert Xu, davem, kaber, yoshfuji, nakam, eric.dumazet, netdev
In-Reply-To: <1267538070.21749.27.camel@bigi>
On Tue, Mar 02, 2010 at 08:54:30AM -0500, jamal wrote:
>
> yikes. Ok. Seems this fix needs to go -stable as well then.
Indeed, it should.
>
> > I tried it, works for me too.
>
> Did you try with CONFIG_XFRM_SUB_POLICY=y. Thats the only reason
> i said "looks like it might work". If you tried with that, then
> I dont need to test and I can add an ACKed-by;->
>
Yes, I tested with CONFIG_XFRM_SUB_POLICY=y.
So for me I can add an
Acked-by: Steffen Klassert <steffen.klassert@secunet.com>
^ permalink raw reply
* Re: [RFC PATCH]xfrm: fix perpetual bundles
From: David Miller @ 2010-03-02 14:06 UTC (permalink / raw)
To: hadi
Cc: steffen.klassert, herbert, kaber, yoshfuji, nakam, eric.dumazet,
netdev
In-Reply-To: <1267538070.21749.27.camel@bigi>
From: jamal <hadi@cyberus.ca>
Date: Tue, 02 Mar 2010 08:54:30 -0500
> On Tue, 2010-03-02 at 14:46 +0100, Steffen Klassert wrote:
>
>> The problem was spotted by
>> commit a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
>> xfrm: select sane defaults for xfrm[4|6] gc_thresh
>>
>> Before this commit, the xfrm garbage collector started to remove
>> stale bundle entries as soon as we reached an amount of 1024
>> bundle entries. Now the default value for the gc_thresh is
>> based on the main route table hash size, so we can have much more
>> bundle entries.
>
> yikes. Ok. Seems this fix needs to go -stable as well then.
Jamal please do some testing then I'll push Herbert's fix around.
^ permalink raw reply
* Re: Gianfar driver failing on MPC8641D based board
From: Anton Vorontsov @ 2010-03-02 14:02 UTC (permalink / raw)
To: Kumar Gopalpet-B05799
Cc: Paul Gortmaker, Martyn Welch, netdev, linux-kernel,
linuxppc-dev list, davem
In-Reply-To: <9F4C7D19E8361D4C94921B95BE08B81BC93EC3@zin33exm22.fsl.freescale.net>
Hi!
On Sat, Feb 27, 2010 at 11:05:32AM +0530, Kumar Gopalpet-B05799 wrote:
[...]
> Understood, and thanks for the explanation. Am I correct in saying that
> this is
> due to the out-of-order execution capability on powerpc ?
Nope, that was just a logic issue in the driver.
Though, with the patch, the eieio() is needed so that compiler (or CPU)
won't reorder lstatus and skbuff writes.
> I have one more question, why don't we use use atomic_t for num_txbdfree
> and
> completely do away with spin_locks in gfar_clean_tx_ring() and
> gfar_start_xmit().
> In an non-SMP, scenario I would feel there is absolutely no requirement
> of spin_locks
> and in case of SMP atomic operation would be much more safer on powerpc
> rather than spin_locks.
>
> What is your suggestion ?
I think that's a good idea.
However, in start_xmit() we'll have to keep the spinlock anyway
since it also protects from gfar_error(), which can modify
regs->tstat.
Thanks!
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH 1/1] bnx2x: Tx barriers and locks
From: Stanislaw Gruszka @ 2010-03-02 13:55 UTC (permalink / raw)
To: Vladislav Zolotarov
Cc: Michael Chan, netdev@vger.kernel.org, davem@davemloft.net,
Eilon Greenstein, Matthew Carlson
In-Reply-To: <8628FE4E7912BF47A96AE7DD7BAC0AADCB46A2B40C@SJEXCHCCR02.corp.ad.broadcom.com>
On Tue, Mar 02, 2010 at 04:50:59AM -0800, Vladislav Zolotarov wrote:
> Stanislaw barrier() is not a memory barrier - it's a compiler barrier. I don't think removing it from bnx2x_tx_avail() will improve anything. If u think I'm wrong, could u, pls., provide a specific example.
Only improvement is removing confusing code, And comment like
"Tell compiler that prod and cons can change" is even more
confusing. If you think I'm wrong, just tell as why that
barrier is needed :)
For bnx2 and tg3 that would be removal of smp_mb().
> Let's recall that the bxn2x_start_xmit() is called under tx_lock(), so no additional memory barriers needed to ensure that bnx2x_tx_avail() has fresh values of BD consumer and producer at the beginning of bnx2x_start_xmit().
Memory barriers are not about refreshing anything, they are
about ordering.
> That's why we don't need the more complicated logic putting the queue asleep at the beginning of bnx2x_start_xmit() as we need at the end. That's why I think your "goto" change in the beginning of bnx2x_start_xmit() is suboptimal.
You may have right, about that smb_mb() is not needed at the beginning
of start_xmit(), but I'm not sure for that, since we are changing tx_bd_cons
without netif_tx_lock and setting XOFF bit there. If we can assume that
at the beginning of start_xmit() there is always enough space it hw queue,
we can just remove tx_avail() check. Otherwise IMHO is better to put queue
asleep in safe way.
About performance, adding smp_mb() on bug/unused code path does not hurt
much :)
Cheers
Stanislaw
> > -----Original Message-----
> > From: Stanislaw Gruszka [mailto:sgruszka@redhat.com]
> > Sent: Tuesday, March 02, 2010 1:31 PM
> > To: Michael Chan
> > Cc: Vladislav Zolotarov; netdev@vger.kernel.org;
> > davem@davemloft.net; Eilon Greenstein; Matthew Carlson
> > Subject: Re: [PATCH 1/1] bnx2x: Tx barriers and locks
> >
> > On Mon, Mar 01, 2010 at 09:59:07AM -0800, Michael Chan wrote:
> > > > There is still difference between what we have in bnx2x
> > and bnx2/tg3
> > > > regarding memory barriers in tx_poll/start_xmit code.
> > Mainly we have
> > > > smp_mb() in bnx2/tg3_tx_avail(), and in bnx2/tg3_tx_int()
> > is smp_mb()
> > > > not smp_wmb(). I do not see that bnx2x is wrong, but
> > would like to know
> > > > why there is a difference, maybe bnx2/tg3 should be changed?
> > > >
> > >
> > > The memory barrier in tx_int() is to make the tx index update happen
> > > before the netif_tx_queue_stopped() check. The barrier is
> > to prevent a
> > > situation like this:
> > >
> > > CPU0 CPU1
> > > start_xmit()
> > > if (tx_ring_full) {
> > > tx_int()
> > >
> > if (!netif_tx_queue_stopped)
> > > netif_tx_stop_queue()
> > > if (!tx_ring_full)
> > >
> > update_tx_index
> > > netif_tx_wake_queue()
> > > }
> > >
> > >
> > > The update_tx_index code is before the if statement in
> > program order,
> > > but the CPU and/or compiler can reorder it as shown above.
> > smp_mb() will
> > > prevent that. Will smp_wmb() prevent that as well?
> >
> > No. smp_wmb() affect only write orders on CPU1 performing tx_int(), so
> > that should be fixed in bnx2x.
> >
> > Regarding memory barrier in tx_avail(), I don't think it its
> > needed for
> > anything, except maybe usage at the beginning of
> > start_xmit(), but we can
> > just remove that like in the patch below. I going to post "official"
> > patches for tg3, bnx2 and bnx2x, if no nobody has nothing against.
> >
> > diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> > index ed785a3..0f406b8 100644
> > --- a/drivers/net/bnx2x_main.c
> > +++ b/drivers/net/bnx2x_main.c
> > @@ -893,7 +893,6 @@ static inline u16 bnx2x_tx_avail(struct
> > bnx2x_fastpath *fp)
> > u16 prod;
> > u16 cons;
> >
> > - barrier(); /* Tell compiler that prod and cons can change */
> > prod = fp->tx_bd_prod;
> > cons = fp->tx_bd_cons;
> >
> > @@ -963,9 +962,8 @@ static int bnx2x_tx_int(struct bnx2x_fastpath *fp)
> > * start_xmit() will miss it and cause the queue to be stopped
> > * forever.
> > */
> > - smp_wmb();
> > + smp_mb();
> >
> > - /* TBD need a thresh? */
> > if (unlikely(netif_tx_queue_stopped(txq))) {
> > /* Taking tx_lock() is needed to prevent
> > reenabling the queue
> > * while it's empty. This could have happen if
> > rx_action() gets
> > @@ -11177,10 +11175,9 @@ static netdev_tx_t
> > bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > struct eth_tx_bd *tx_data_bd, *total_pkt_bd = NULL;
> > struct eth_tx_parse_bd *pbd = NULL;
> > u16 pkt_prod, bd_prod;
> > - int nbd, fp_index;
> > + int nbd, fp_index, i, ret;
> > dma_addr_t mapping;
> > u32 xmit_type = bnx2x_xmit_type(bp, skb);
> > - int i;
> > u8 hlen = 0;
> > __le16 pkt_size = 0;
> >
> > @@ -11195,10 +11192,9 @@ static netdev_tx_t
> > bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > fp = &bp->fp[fp_index];
> >
> > if (unlikely(bnx2x_tx_avail(fp) <
> > (skb_shinfo(skb)->nr_frags + 3))) {
> > - fp->eth_q_stats.driver_xoff++;
> > - netif_tx_stop_queue(txq);
> > BNX2X_ERR("BUG! Tx ring full when queue awake!\n");
> > - return NETDEV_TX_BUSY;
> > + ret = NETDEV_TX_BUSY;
> >
> > + goto stop_queue;
> > }
> >
> > DP(NETIF_MSG_TX_QUEUED, "SKB: summed %x protocol %x
> > protocol(%x,%x)"
> > @@ -11426,19 +11422,24 @@ static netdev_tx_t
> > bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > mmiowb();
> >
> > fp->tx_bd_prod += nbd;
> > -
> > - if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) {
> > - netif_tx_stop_queue(txq);
> > - /* We want bnx2x_tx_int to "see" the updated tx_bd_prod
> > - if we put Tx into XOFF state. */
> > - smp_mb();
> > - fp->eth_q_stats.driver_xoff++;
> > - if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3)
> > - netif_tx_wake_queue(txq);
> > - }
> > fp->tx_pkt++;
> > +
> > + ret = NETDEV_TX_OK;
> > + if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3))
> > + goto stop_queue;
> > +
> > + return ret;
> >
> > - return NETDEV_TX_OK;
> > +stop_queue:
> > + netif_tx_stop_queue(txq);
> > + /* paired barrier is in bnx2x_tx_int(), update of tx_bd_cons
> > + * have to be visable here, after we XOFF bit setting */
> > + smp_mb();
> > + fp->eth_q_stats.driver_xoff++;
> > + if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3)
> > + netif_tx_wake_queue(txq);
> > +
> > + return ret;
> > }
> >
> > /* called with rtnl_lock */
> >
> >
^ permalink raw reply
* Re: [RFC PATCH]xfrm: fix perpetual bundles
From: jamal @ 2010-03-02 13:54 UTC (permalink / raw)
To: Steffen Klassert
Cc: Herbert Xu, davem, kaber, yoshfuji, nakam, eric.dumazet, netdev
In-Reply-To: <20100302134656.GC20508@secunet.com>
On Tue, 2010-03-02 at 14:46 +0100, Steffen Klassert wrote:
> The problem was spotted by
> commit a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
> xfrm: select sane defaults for xfrm[4|6] gc_thresh
>
> Before this commit, the xfrm garbage collector started to remove
> stale bundle entries as soon as we reached an amount of 1024
> bundle entries. Now the default value for the gc_thresh is
> based on the main route table hash size, so we can have much more
> bundle entries.
yikes. Ok. Seems this fix needs to go -stable as well then.
> I tried it, works for me too.
Did you try with CONFIG_XFRM_SUB_POLICY=y. Thats the only reason
i said "looks like it might work". If you tried with that, then
I dont need to test and I can add an ACKed-by;->
cheers,
jamal
^ permalink raw reply
* Re: [RFC PATCH]xfrm: fix perpetual bundles
From: Steffen Klassert @ 2010-03-02 13:46 UTC (permalink / raw)
To: jamal; +Cc: Herbert Xu, davem, kaber, yoshfuji, nakam, eric.dumazet, netdev
In-Reply-To: <1267535426.21749.24.camel@bigi>
On Tue, Mar 02, 2010 at 08:10:26AM -0500, jamal wrote:
> On Tue, 2010-03-02 at 20:51 +0800, Herbert Xu wrote:
>
> > Heh, you've just discovered a bug that I carefully planted back
> > in 2007, while merging the v4/v6 policy code :)
>
> ;-> I am suprised it hasnt been noticed sooner given it accumulates
> memory on a per-packet basis.
The problem was spotted by
commit a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
xfrm: select sane defaults for xfrm[4|6] gc_thresh
Before this commit, the xfrm garbage collector started to remove
stale bundle entries as soon as we reached an amount of 1024
bundle entries. Now the default value for the gc_thresh is
based on the main route table hash size, so we can have much more
bundle entries.
>
> > It is a clear merging error, where *fl became rt->fl which is
> > totally different. So please try this patch:
> >
>
> Looks like it would work.
> I dont have time right now - but will by either tonight or tomorrow
> evening. Steffen, if you have time - please go ahead and try it out
> as well.
>
I tried it, works for me too.
Thanks,
Steffen
^ permalink raw reply
* Re: Panic at tcp_xmit_retransmit_queue
From: sbs @ 2010-03-02 13:16 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Netdev, LKML
In-Reply-To: <alpine.DEB.2.00.1002151520250.7063@wel-95.cs.helsinki.fi>
thank you very much, have stable running server for a week and it
seems that it works like a charm now, i havent detected any panics
since i apply the patch. although seems that the problem stops
ocurring cause i dont see any debug information through netconsole
On Mon, Feb 15, 2010 at 4:21 PM, Ilpo Järvinen
<ilpo.jarvinen@helsinki.fi> wrote:
> On Wed, 3 Feb 2010, Ilpo Järvinen wrote:
>
>> On Mon, 1 Feb 2010, sbs wrote:
>>
>> > actually removing netconsole from kernel didnt help.
>> > i found many guys with the same problem but with different hardware
>> > configurations here:
>> >
>> > freez in TCP stack :
>> > http://bugzilla.kernel.org/show_bug.cgi?id=14470
>> >
>> > is there someone who can investigate it?
>> >
>> >
>> > On Tue, Jan 19, 2010 at 7:13 PM, sbs <gexlie@gmail.com> wrote:
>> > > We are hiting kernel panics on servers with nVidia MCP55 NICs once a day;
>> > > it appears usualy under a high network trafic ( around 10000Mbit/s) but
>> > > it is not a rule, it has happened even on low trafic.
>> > >
>> > > Servers are used as nginx+static content
>> > > On 2 equal servers this panic happens aprox 2 times a day depending on
>> > > network load. Machine completly freezes till the netconsole reboots.
>> > >
>> > > Kernel: 2.6.32.3
>> > >
>> > > what can it be? whats wrong with tcp_xmit_retransmit_queue() function ?
>> > > can anyone explain or fix?
>>
>> You might want to try with to debug patch below. It might even make the
>> box to survive the event (if I got it coded right).
>
> Here should be a better version of the debug patch, hopefully the infinite
> looping is now gone.
>
> --
> i.
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 383ce23..4672a30 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2186,6 +2186,42 @@ static int tcp_can_forward_retransmit(struct sock *sk)
> return 1;
> }
>
> +static void print_queue(struct sock *sk, struct sk_buff *old, struct sk_buff *hole)
> +{
> + struct tcp_sock *tp = tcp_sk(sk);
> + struct sk_buff *skb, *prev;
> +
> + skb = tcp_write_queue_head(sk);
> + prev = (struct sk_buff *)(&sk->sk_write_queue);
> +
> + if (skb == NULL) {
> + printk("NULL head, pkts %u\n", tp->packets_out);
> + return;
> + }
> + printk("head %p tail %p sendhead %p oldhint %p now %p hole %p high %u\n",
> + tcp_write_queue_head(sk), tcp_write_queue_tail(sk),
> + tcp_send_head(sk), old, tp->retransmit_skb_hint, hole,
> + tp->retransmit_high);
> +
> + while (skb) {
> + printk("skb %p (%u-%u) next %p prev %p sacked %u\n",
> + skb, TCP_SKB_CB(skb)->seq, TCP_SKB_CB(skb)->end_seq,
> + skb->next, skb->prev, TCP_SKB_CB(skb)->sacked);
> + if (prev != skb->prev)
> + printk("Inconsistent prev\n");
> +
> + if (skb == tcp_write_queue_tail(sk)) {
> + if (skb->next != (struct sk_buff *)(&sk->sk_write_queue))
> + printk("Improper next at tail\n");
> + return;
> + }
> +
> + prev = skb;
> + skb = skb->next;
> + }
> + printk("Encountered unexpected NULL\n");
> +}
> +
> /* This gets called after a retransmit timeout, and the initially
> * retransmitted data is acknowledged. It tries to continue
> * resending the rest of the retransmit queue, until either
> @@ -2194,12 +2230,15 @@ static int tcp_can_forward_retransmit(struct sock *sk)
> * based retransmit packet might feed us FACK information again.
> * If so, we use it to avoid unnecessarily retransmissions.
> */
> +static int caught_it = 0;
> +
> void tcp_xmit_retransmit_queue(struct sock *sk)
> {
> const struct inet_connection_sock *icsk = inet_csk(sk);
> struct tcp_sock *tp = tcp_sk(sk);
> struct sk_buff *skb;
> struct sk_buff *hole = NULL;
> + struct sk_buff *old = tp->retransmit_skb_hint;
> u32 last_lost;
> int mib_idx;
> int fwd_rexmitting = 0;
> @@ -2217,6 +2256,16 @@ void tcp_xmit_retransmit_queue(struct sock *sk)
> last_lost = tp->snd_una;
> }
>
> +checknull:
> + if (skb == NULL) {
> + if (!caught_it)
> + print_queue(sk, old, hole);
> + caught_it++;
> + if (net_ratelimit())
> + printk("Errors caught so far %u\n", caught_it);
> + return;
> + }
> +
> tcp_for_write_queue_from(skb, sk) {
> __u8 sacked = TCP_SKB_CB(skb)->sacked;
>
> @@ -2257,7 +2306,7 @@ begin_fwd:
> } else if (!(sacked & TCPCB_LOST)) {
> if (hole == NULL && !(sacked & (TCPCB_SACKED_RETRANS|TCPCB_SACKED_ACKED)))
> hole = skb;
> - continue;
> + goto cont;
>
> } else {
> last_lost = TCP_SKB_CB(skb)->end_seq;
> @@ -2268,7 +2317,7 @@ begin_fwd:
> }
>
> if (sacked & (TCPCB_SACKED_ACKED|TCPCB_SACKED_RETRANS))
> - continue;
> + goto cont;
>
> if (tcp_retransmit_skb(sk, skb))
> return;
> @@ -2278,6 +2327,9 @@ begin_fwd:
> inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
> inet_csk(sk)->icsk_rto,
> TCP_RTO_MAX);
> +cont:
> + skb = skb->next;
> + goto checknull;
> }
> }
>
^ permalink raw reply
* Re: [RFC PATCH]xfrm: fix perpetual bundles
From: jamal @ 2010-03-02 13:10 UTC (permalink / raw)
To: Herbert Xu
Cc: davem, kaber, yoshfuji, nakam, eric.dumazet, netdev,
Steffen Klassert
In-Reply-To: <20100302125156.GA2420@gondor.apana.org.au>
On Tue, 2010-03-02 at 20:51 +0800, Herbert Xu wrote:
> Heh, you've just discovered a bug that I carefully planted back
> in 2007, while merging the v4/v6 policy code :)
;-> I am suprised it hasnt been noticed sooner given it accumulates
memory on a per-packet basis.
> It is a clear merging error, where *fl became rt->fl which is
> totally different. So please try this patch:
>
Looks like it would work.
I dont have time right now - but will by either tonight or tomorrow
evening. Steffen, if you have time - please go ahead and try it out
as well.
cheers,
jamal
^ permalink raw reply
* Re: [RFC][PATCH] ns: Syscalls for better namespace sharing control.
From: Cedric Le Goater @ 2010-03-02 13:10 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Daniel Lezcano, Pavel Emelyanov, Linux Netdev List, containers,
Netfilter Development Mailinglist, Ben Greear
In-Reply-To: <m1ljebwwgd.fsf@fess.ebiederm.org>
On 03/01/2010 10:42 PM, Eric W. Biederman wrote:
> I am of two mind about my patches. Right now they are a brilliant
> proof of concept that we can name namespaces without needing a
> namespace for the names of namespaces, and start to be a practical
> solution to the join problem. At the same time, I'm not certain
> I like a solution that requires yet more syscalls so I ask myself
> is there not yet a simpler way.
thinking aloud,
what if you made the nsproxy a vfs_inode ? we could then mount the nsfs
to do all sorts of fs operations on the object, like notifying easily
its deletion. we would need to find a meaningful name, probably the inode
number.
one syscall (nsfd) would be required to get the nsproxy of a task (pid).
you can't guess that from an inode number.
C.
^ 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