Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH 1/2] net: davinci_mdio: enable and disable clock
From: Daniel Mack @ 2012-08-02 20:17 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: netdev, mugunthanvnm, paul, devicetree-discuss, koen,
	linux-arm-kernel
In-Reply-To: <20120802195308.GZ6802@n2100.arm.linux.org.uk>

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On 02.08.2012 21:53, Russell King - ARM Linux wrote:
> On Thu, Aug 02, 2012 at 09:43:35PM +0200, Daniel Mack wrote:
>> Make the driver control the device clocks. Appearantly, the Davinci
>> platform probes this driver with the clock all powered up, but on OMAP,
>> this isn't the case.
> 
> Hmm, this looks like it could do with improvement, especially as we're
> moving everything over to a common clk API.
> 
> 1. This driver could do with clk_prepare()/clk_unprepare() calls.

Ok, done.

> 2. This driver should not be making the assumption that NULL means
>    it can avoid clk_* calls.  It should instead be using
> 	if (!IS_ERR(clk))

Well spotted. Amended patch below.


Thanks,
Daniel




[-- Attachment #2: 0001-net-davinci_mdio-prepare-and-unprepare-clocks.patch --]
[-- Type: text/x-patch, Size: 2045 bytes --]

>From 57670e52d19218f897d835d25223bf4b4932252f Mon Sep 17 00:00:00 2001
From: Daniel Mack <zonque@gmail.com>
Date: Thu, 2 Aug 2012 21:24:36 +0200
Subject: [PATCH] net: davinci_mdio: prepare and unprepare clocks

Make the driver control the device clocks. Appearantly, the Davinci
platform probes this driver with the clock all powered up, but on OMAP,
this isn't the case.

While at it, also check for IS_ERR(data->clk) in the bail_out: label of
.probe().

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/net/ethernet/ti/davinci_mdio.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index cd7ee20..462f81d 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -332,6 +332,8 @@ static int __devinit davinci_mdio_probe(struct platform_device *pdev)
 		goto bail_out;
 	}
 
+	clk_prepare(data->clk);
+
 	dev_set_drvdata(dev, data);
 	data->dev = dev;
 	spin_lock_init(&data->lock);
@@ -379,8 +381,11 @@ bail_out:
 	if (data->bus)
 		mdiobus_free(data->bus);
 
-	if (data->clk)
+	if (data->clk && !IS_ERR(data->clk)) {
+		clk_unprepare(data->clk);
 		clk_put(data->clk);
+	}
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -397,8 +402,11 @@ static int __devexit davinci_mdio_remove(struct platform_device *pdev)
 	if (data->bus)
 		mdiobus_free(data->bus);
 
-	if (data->clk)
+	if (data->clk) {
+		clk_unprepare(data->clk);
 		clk_put(data->clk);
+	}
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -427,6 +435,8 @@ static int davinci_mdio_suspend(struct device *dev)
 	data->suspended = true;
 	spin_unlock(&data->lock);
 
+	clk_unprepare(data->clk);
+
 	return 0;
 }
 
@@ -435,6 +445,8 @@ static int davinci_mdio_resume(struct device *dev)
 	struct davinci_mdio_data *data = dev_get_drvdata(dev);
 	u32 ctrl;
 
+	clk_prepare(data->clk);
+
 	spin_lock(&data->lock);
 	pm_runtime_put_sync(data->dev);
 
-- 
1.7.11.2


^ permalink raw reply related

* Re: [PATCH 1/2] net: davinci_mdio: enable and disable clock
From: Russell King - ARM Linux @ 2012-08-02 19:53 UTC (permalink / raw)
  To: Daniel Mack
  Cc: netdev, mugunthanvnm, paul, devicetree-discuss, koen,
	linux-arm-kernel
In-Reply-To: <1343936616-29318-1-git-send-email-zonque@gmail.com>

On Thu, Aug 02, 2012 at 09:43:35PM +0200, Daniel Mack wrote:
> Make the driver control the device clocks. Appearantly, the Davinci
> platform probes this driver with the clock all powered up, but on OMAP,
> this isn't the case.

Hmm, this looks like it could do with improvement, especially as we're
moving everything over to a common clk API.

1. This driver could do with clk_prepare()/clk_unprepare() calls.
2. This driver should not be making the assumption that NULL means
   it can avoid clk_* calls.  It should instead be using
	if (!IS_ERR(clk))

Thanks.

^ permalink raw reply

* [PATCH 2/2] net: davinci_mdio: add DT bindings
From: Daniel Mack @ 2012-08-02 19:43 UTC (permalink / raw)
  To: netdev
  Cc: devicetree-discuss, koen, mugunthanvnm, linux-arm-kernel, paul,
	Daniel Mack
In-Reply-To: <1343936616-29318-1-git-send-email-zonque@gmail.com>

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 .../devicetree/bindings/net/davinci_mdio.txt       | 24 +++++++++++++
 drivers/net/ethernet/ti/davinci_mdio.c             | 39 ++++++++++++++++++++++
 2 files changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/davinci_mdio.txt

diff --git a/Documentation/devicetree/bindings/net/davinci_mdio.txt b/Documentation/devicetree/bindings/net/davinci_mdio.txt
new file mode 100644
index 0000000..03292a5
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/davinci_mdio.txt
@@ -0,0 +1,24 @@
+Davinci MDIO DT bindings
+
+Required properties:
+
+ - compatible : should be "ti,davinci-mdio"
+
+Optional properties:
+
+ - bus-freq : an integer to specify the bus speed
+
+Examples:
+
+	mdio: davinci_mdio@4a101000 {
+		compatible = "ti,davinci-mdio";
+		ti,hwmods = "davinci_mdio";
+	};
+
+ or
+
+	mdio: davinci_mdio@4a101000 {
+		compatible = "ti,davinci-mdio";
+		reg = <0x4a101000 0x100>;
+	};
+
diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index b4b6015..2640cae 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -36,6 +36,8 @@
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
 #include <linux/davinci_emac.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 
 /*
  * This timeout definition is a worst-case ultra defensive measure against
@@ -289,6 +291,37 @@ static int davinci_mdio_write(struct mii_bus *bus, int phy_id,
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id davinci_mdio_dt_ids[] = {
+	{ .compatible = "ti,davinci-mdio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, davinci_mdio_dt_ids);
+
+static inline int davinci_mdio_probe_dt(struct device *dev,
+					struct mdio_platform_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+	const struct of_device_id *of_id =
+			of_match_device(davinci_mdio_dt_ids, dev);
+	u32 tmp;
+
+	if (!of_id)
+		return 0;
+
+	if (of_property_read_u32(np, "bus-freq", &tmp) == 0)
+		pdata->bus_freq = tmp;
+
+	return 0;
+}
+#else
+static inline int davinci_mdio_probe_dt(struct device *dev,
+					struct mdio_platform_data *pdata)
+{
+	return 0;
+}
+#endif
+
 static int __devinit davinci_mdio_probe(struct platform_device *pdev)
 {
 	struct mdio_platform_data *pdata = pdev->dev.platform_data;
@@ -306,6 +339,10 @@ static int __devinit davinci_mdio_probe(struct platform_device *pdev)
 
 	data->pdata = pdata ? (*pdata) : default_pdata;
 
+	ret = davinci_mdio_probe_dt(dev, pdata);
+	if (ret < 0)
+		goto free_mem;
+
 	data->bus = mdiobus_alloc();
 	if (!data->bus) {
 		dev_err(dev, "failed to alloc mii bus\n");
@@ -389,6 +426,7 @@ bail_out:
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+free_mem:
 	kfree(data);
 
 	return ret;
@@ -471,6 +509,7 @@ static struct platform_driver davinci_mdio_driver = {
 		.name	 = "davinci_mdio",
 		.owner	 = THIS_MODULE,
 		.pm	 = &davinci_mdio_pm_ops,
+		.of_match_table = of_match_ptr(davinci_mdio_dt_ids),
 	},
 	.probe = davinci_mdio_probe,
 	.remove = __devexit_p(davinci_mdio_remove),
-- 
1.7.11.2

^ permalink raw reply related

* [PATCH 1/2] net: davinci_mdio: enable and disable clock
From: Daniel Mack @ 2012-08-02 19:43 UTC (permalink / raw)
  To: netdev
  Cc: devicetree-discuss, koen, mugunthanvnm, linux-arm-kernel, paul,
	Daniel Mack

Make the driver control the device clocks. Appearantly, the Davinci
platform probes this driver with the clock all powered up, but on OMAP,
this isn't the case.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/net/ethernet/ti/davinci_mdio.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index cd7ee20..b4b6015 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -332,6 +332,8 @@ static int __devinit davinci_mdio_probe(struct platform_device *pdev)
 		goto bail_out;
 	}
 
+	clk_enable(data->clk);
+
 	dev_set_drvdata(dev, data);
 	data->dev = dev;
 	spin_lock_init(&data->lock);
@@ -379,8 +381,11 @@ bail_out:
 	if (data->bus)
 		mdiobus_free(data->bus);
 
-	if (data->clk)
+	if (data->clk) {
+		clk_disable(data->clk);
 		clk_put(data->clk);
+	}
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -397,8 +402,11 @@ static int __devexit davinci_mdio_remove(struct platform_device *pdev)
 	if (data->bus)
 		mdiobus_free(data->bus);
 
-	if (data->clk)
+	if (data->clk) {
+		clk_disable(data->clk);
 		clk_put(data->clk);
+	}
+
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
@@ -427,6 +435,8 @@ static int davinci_mdio_suspend(struct device *dev)
 	data->suspended = true;
 	spin_unlock(&data->lock);
 
+	clk_disable(data->clk);
+
 	return 0;
 }
 
@@ -435,6 +445,8 @@ static int davinci_mdio_resume(struct device *dev)
 	struct davinci_mdio_data *data = dev_get_drvdata(dev);
 	u32 ctrl;
 
+	clk_enable(data->clk);
+
 	spin_lock(&data->lock);
 	pm_runtime_put_sync(data->dev);
 
-- 
1.7.11.2

^ permalink raw reply related

* Re: [PATCH 1/4] net/mlx4_en: Setting the NETIF_F_GRO flag back to dev->hw_features
From: Michał Mirosław @ 2012-08-02 19:36 UTC (permalink / raw)
  To: Yevgeny Petrilin; +Cc: davem, netdev
In-Reply-To: <1343921456-19180-2-git-send-email-yevgenyp@mellanox.co.il>

2012/8/2 Yevgeny Petrilin <yevgenyp@mellanox.co.il>:
> The bug which removed it was introduced in commit c8c64cff
> which added the hw_features.
[...]
> -       dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> +       dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> +                          NETIF_F_GRO;

Just try to realize the truth: there is no bug.

register_netdevice() is setting GSO and GRO bits for everyone.

Best Regards,
Michał Mirosław

^ permalink raw reply

* discussion questions: SR-IOV, virtualization, and bonding
From: Chris Friesen @ 2012-08-02 19:21 UTC (permalink / raw)
  To: e1000-devel@lists.sourceforge.net, netdev


Hi all,

I wanted to just highlight some issues that we're seeing and see what 
others are doing in this area.

Our configuration is that we have a host with SR-IOV-capable NICs with 
bonding enabled on the PF.  Depending on the exact system it could be 
active/standby or some form of active/active.

In the guests we generally have several VFs (corresponding to several 
PFs) and we want to bond them for reliability.

We're seeing a number of issues:

1) If the guests use arp monitoring then broadcast arp packets from the 
guests are visible on the other guests and on the host, and can cause 
them to think the link is good even if we aren't receiving arp packets 
from the external network.  (I'm assuming carrier is up.)

2) If both the host and guest use active/backup but pick different 
devices as the active, there is no traffic between host/guest over the 
bond link.  Packets are sent out the active and looped back internally 
to arrive on the inactive, then skb_bond_should_drop() suppresses them.

3) For active/standby the default is to set the standby to the MAC 
address of the bond.  If the host has already set the MAC address (using 
some algorithm to ensure uniqueness within the local network) then the 
guest is not allowed to change it.


So far the solutions to 1 seem to be either using arp validation (which 
currently doesn't exist for loadbalancing modes) or else have the 
underlying ethernet driver distinguish between packets coming from the 
wire vs being looped back internally and have the bonding driver only 
set last_rx for external packets.

For issue 2, it would seem beneficial for the host to be able to ensure 
that the guest uses the same link as the active.  I don't see a tidy 
solution here.  One somewhat messy possibility here is to have bonding 
send a message to the standby PF which then tells all its VFs to fake 
loss of carrier.

For issue 3, the logical solution would seem to be some way of assigning 
a list of "valid" mac addresses to a given VF--like maybe all MAC 
addresses assigned to a VM or something.  Anyone have any bright ideas?


I'm sure we're not the only ones running into this, so what are others 
doing?  Is the only current option to use active/active with miimon?

Chris

-- 

Chris Friesen
Software Designer

3500 Carling Avenue
Ottawa, Ontario K2H 8E9
www.genband.com

^ permalink raw reply

* Re: binding UDP port 0 with SO_REUSEADDR
From: Tobias S. Josefowitz @ 2012-08-02 19:05 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1343926645.9299.303.camel@edumazet-glaptop>

On Thu, Aug 2, 2012 at 6:57 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:

> There is no value using 'port 0' and REUSEADDR on UDP, really.

And nobody ever makes mistakes, so why not surprise them for even more
than they are in for.

"Looking into src/pkg/net/sock.go (func socket) I see that SO_REUSEADDR
is set unconditionally to all types of sockets." --
http://code.google.com/p/go/issues/detail?id=1692

Anyway, I'm out of this now.

Tobi

^ permalink raw reply

* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Eric W. Biederman @ 2012-08-02 17:15 UTC (permalink / raw)
  To: Or Gerlitz; +Cc: davem, roland, netdev, ali, sean.hefty, Erez Shitrit
In-Reply-To: <1343840975-3252-10-git-send-email-ogerlitz@mellanox.com>

Or Gerlitz <ogerlitz@mellanox.com> writes:

> From: Erez Shitrit <erezsh@mellanox.co.il>
>
> The eipoib driver provides a standard Ethernet netdevice over
> the InfiniBand IPoIB interface .
>
> Some services can run only on top of Ethernet L2 interfaces, and cannot be
> bound to an IPoIB interface. With this new driver, these services can run
> seamlessly.

Do I read this code correctly that what you are doing is not tunneling
ethernet over IB but instead you are removing an ethernet header and
replacing it with an IB header?

Do I also read this code correctly if you can't find your destination
mac address in your ""neighbor table"" you do a normal IPoIB arp
for the infiniband GUID?

Do I read this right that if presented with a non-IPv4 or ARP packet
this code will do something undefined and unpredictable?

Maybe this makes some sense but just skimming it looks like you
are trying to force a square peg into a round hole resulting in
some weird code and some very weird maintainability issues.

I am honestly surprised at this approach.  I would think it would be
faster and simpler to run an IB queue pair directly to the hypervisor or
possibly even the guest operating system bypassing the kernel and doing
all of this translation in userspace.

Eric

^ permalink raw reply

* Re: [PATCH v2] dynamic_debug: Restore dev_dbg functionality, optimize stack
From: Joe Perches @ 2012-08-02 17:02 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Greg Kroah-Hartman, David S. Miller, Jason Baron, Jim Cromie,
	Kay Sievers, linux-kernel, netdev
In-Reply-To: <20120731145534.5606e840.akpm@linux-foundation.org>

On Tue, 2012-07-31 at 14:55 -0700, Andrew Morton wrote:
> On Sat, 28 Jul 2012 00:55:07 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > commit c4e00daaa9 ("driver-core: extend dev_printk() to pass structured data")
> > changed __dev_printk and broke dynamic-debug's ability to control the
> > dynamic prefix of dev_dbg(dev,..).
> > 
> > dynamic_emit_prefix() adds "[tid] module:func:line:" to the output and
> > those additions got lost.
> > 
> > In addition, the current dynamic debug code uses up to 3 recursion
> > levels via %pV.  This can consume quite a bit of stack.  Directly
> > call printk_emit to reduce the recursion depth.
[]
> > Changes in v2:
> > 
> > o Fix dynamic_emit_prefix to always initialize output
> > o Call create_syslog_header and emit_printk from__netdev_printk and
> >   eliminate call to dev_printk to remove another recursion via %pV
> 
> This one causes an oops-on-boot
> (http://ozlabs.org/~akpm/stuff/IMG_20120731_144047.jpg).  v1 didn't do
> that.
> 
> config: http://ozlabs.org/~akpm/stuff/config-akpm2
> 

Thanks.  I'll fix it when I can.
Expect a week or so delay though.

I'm a bit busy on other projects and this one seems to
require additional testing.

cheers, Joe

^ permalink raw reply

* Re: binding UDP port 0 with SO_REUSEADDR
From: Eric Dumazet @ 2012-08-02 16:57 UTC (permalink / raw)
  To: Tobias S. Josefowitz; +Cc: netdev
In-Reply-To: <CAEZM=bM=AGp=FOxLiDiBFgVJWGmNdPpG_FV3Ne2i5jeALiB4YA@mail.gmail.com>

On Thu, 2012-08-02 at 18:33 +0200, Tobias S. Josefowitz wrote:
> Hi again,
> 
> I need to say, after looking at "my" socket(7),
> 
>        SO_REUSEADDR
>               Indicates  that  the rules used in validating addresses supplied
>               in a bind(2) call should allow reuse of  local  addresses.   For
>               AF_INET  sockets  this means that a socket may bind, except when
>               there is an active listening socket bound to the address.   When
>               the listening socket is bound to INADDR_ANY with a specific port
>               then it is not possible to bind  to  this  port  for  any  local
>               address.  Argument is an integer boolean flag.
> 
> I think the surprise-factor of the bind-0-behaviour even increased for
> me. Is there a specific reason for handing out used ports when binding
> port 0 with REUSEADDR?

There is no concept of listening sockets for UDP.

This documentation applies for TCP, and makes no sense for UDP.

There is no value using 'port 0' and REUSEADDR on UDP, really.

^ permalink raw reply

* Re: binding UDP port 0 with SO_REUSEADDR
From: Tobias S. Josefowitz @ 2012-08-02 16:33 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev
In-Reply-To: <CAEZM=bPqOgRUvUKfdYkrVXoWin+SMft0GK67hqsGhBD6nocryA@mail.gmail.com>

Hi again,

I need to say, after looking at "my" socket(7),

       SO_REUSEADDR
              Indicates  that  the rules used in validating addresses supplied
              in a bind(2) call should allow reuse of  local  addresses.   For
              AF_INET  sockets  this means that a socket may bind, except when
              there is an active listening socket bound to the address.   When
              the listening socket is bound to INADDR_ANY with a specific port
              then it is not possible to bind  to  this  port  for  any  local
              address.  Argument is an integer boolean flag.

I think the surprise-factor of the bind-0-behaviour even increased for
me. Is there a specific reason for handing out used ports when binding
port 0 with REUSEADDR?

Best,

Tobi

^ permalink raw reply

* RE: [PATCH net-next] igb: use build_skb()
From: Wyborny, Carolyn @ 2012-08-02 16:10 UTC (permalink / raw)
  To: Eric Dumazet, Kirsher, Jeffrey T; +Cc: Duyck, Alexander H, netdev
In-Reply-To: <1343922692.9299.231.camel@edumazet-glaptop>

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Eric Dumazet
Sent: Thursday, August 02, 2012 8:52 AM
To: Kirsher, Jeffrey T
Cc: Duyck, Alexander H; netdev
Subject: [PATCH net-next] igb: use build_skb()

From: Eric Dumazet <edumazet@google.com>

By using netdev_alloc_frag() & build_skb() instead of legacy
netdev_alloc_skb_ip_align() calls, we reduce number of cache misses in RX path and size of working set.

For a given rx workload, number of 'inuse' sk_buff can be reduced to a very minimum, especially when packets are dropped by our stack.

(Before this patch, default sk_buff allocation was 2048 sk_buffs in rx ring buffer)

They are initialized right before being delivered to stack, so can stay hot in cpu caches.

Ethernet header prefetching is more effective (old prefetch of skb->data paid a stall to access skb->data pointer)

I have 15% performance increase in a RX stress test, removing SLUB slow path in the profiles.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
---

Thanks Eric, 

We've been working on some more extensive performance refactoring that will be coming in addition to this.

Jeff will pull these patches into his tree for testing.

Carolyn

Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation




^ permalink raw reply

* Re: [PATCH] igb: reduce Rx header size
From: Eric Dumazet @ 2012-08-02 15:52 UTC (permalink / raw)
  To: Alexander Duyck; +Cc: Jeff Kirsher, netdev, Wyborny, Carolyn
In-Reply-To: <501AA00E.3010400@intel.com>

On Thu, 2012-08-02 at 08:43 -0700, Alexander Duyck wrote:

> Would there be any problems with us running this through Jeff's tree for
> testing prior to applying this?  I'm just wanting to do some testing
> internally for any performance regressions due to this change.

Sure, I was expecting this exactly, sorry if it was not clear.

( by the way, I am more interested on the second patch I just sent )

Thanks !

^ permalink raw reply

* [PATCH net-next] igb: use build_skb()
From: Eric Dumazet @ 2012-08-02 15:51 UTC (permalink / raw)
  To: Jeff Kirsher; +Cc: Alexander Duyck, netdev

From: Eric Dumazet <edumazet@google.com>

By using netdev_alloc_frag() & build_skb() instead of legacy
netdev_alloc_skb_ip_align() calls, we reduce number of cache misses in
RX path and size of working set.

For a given rx workload, number of 'inuse' sk_buff can be reduced to a
very minimum, especially when packets are dropped by our stack.

(Before this patch, default sk_buff allocation was 2048 sk_buffs in rx
ring buffer)

They are initialized right before being delivered to stack, so can stay
hot in cpu caches.

Ethernet header prefetching is more effective (old prefetch of skb->data
paid a stall to access skb->data pointer)

I have 15% performance increase in a RX stress test, removing SLUB slow
path in the profiles.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
---
 drivers/net/ethernet/intel/igb/igb.h         |    8 ++
 drivers/net/ethernet/intel/igb/igb_ethtool.c |   14 ++--
 drivers/net/ethernet/intel/igb/igb_main.c    |   56 ++++++++++-------
 3 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 9e572dd..e43b160 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -135,6 +135,11 @@ struct vf_data_storage {
 #define IGB_RXBUFFER_16384 16384
 #define IGB_RX_HDR_LEN     IGB_RXBUFFER_512
 
+/* size of fragments allocated with netdev_alloc_frag() */
+#define IGB_FRAGSZ							\
+	(SKB_DATA_ALIGN(IGB_RX_HDR_LEN + NET_SKB_PAD + NET_IP_ALIGN) +	\
+	 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))		
+
 /* How many Tx Descriptors do we need to call netif_wake_queue ? */
 #define IGB_TX_QUEUE_WAKE	16
 /* How many Rx Buffers do we bundle into one write to the hardware ? */
@@ -173,7 +178,7 @@ struct igb_tx_buffer {
 };
 
 struct igb_rx_buffer {
-	struct sk_buff *skb;
+	void *data;
 	dma_addr_t dma;
 	struct page *page;
 	dma_addr_t page_dma;
@@ -242,6 +247,7 @@ struct igb_ring {
 	u16 next_to_clean ____cacheline_aligned_in_smp;
 	u16 next_to_use;
 
+	struct sk_buff *skb;
 	union {
 		/* TX */
 		struct {
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index a19c84c..e6fc5cb 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -1636,12 +1636,12 @@ static void igb_create_lbtest_frame(struct sk_buff *skb,
 	memset(&skb->data[frame_size + 12], 0xAF, 1);
 }
 
-static int igb_check_lbtest_frame(struct sk_buff *skb, unsigned int frame_size)
+static int igb_check_lbtest_frame(const u8 *data, unsigned int frame_size)
 {
 	frame_size /= 2;
-	if (*(skb->data + 3) == 0xFF) {
-		if ((*(skb->data + frame_size + 10) == 0xBE) &&
-		   (*(skb->data + frame_size + 12) == 0xAF)) {
+	if (*(data + 3) == 0xFF) {
+		if ((*(data + frame_size + 10) == 0xBE) &&
+		   (*(data + frame_size + 12) == 0xAF)) {
 			return 0;
 		}
 	}
@@ -1675,8 +1675,10 @@ static int igb_clean_test_rings(struct igb_ring *rx_ring,
 				 DMA_FROM_DEVICE);
 		rx_buffer_info->dma = 0;
 
-		/* verify contents of skb */
-		if (!igb_check_lbtest_frame(rx_buffer_info->skb, size))
+		/* verify contents of buffer */
+		if (!igb_check_lbtest_frame(rx_buffer_info->data +
+					    NET_SKB_PAD + NET_IP_ALIGN,
+					    size))
 			count++;
 
 		/* unmap buffer on tx side */
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b7c2d50..8b732c9 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -538,14 +538,14 @@ rx_ring_summary:
 					"--------- %p%s\n", "RWB", i,
 					le64_to_cpu(u0->a),
 					le64_to_cpu(u0->b),
-					buffer_info->skb, next_desc);
+					buffer_info->data, next_desc);
 			} else {
 				pr_info("%s[0x%03X]     %016llX %016llX %016llX"
 					" %p%s\n", "R  ", i,
 					le64_to_cpu(u0->a),
 					le64_to_cpu(u0->b),
 					(u64)buffer_info->dma,
-					buffer_info->skb, next_desc);
+					buffer_info->data, next_desc);
 
 				if (netif_msg_pktdata(adapter)) {
 					print_hex_dump(KERN_INFO, "",
@@ -3379,7 +3379,7 @@ static void igb_clean_rx_ring(struct igb_ring *rx_ring)
 	if (!rx_ring->rx_buffer_info)
 		return;
 
-	/* Free all the Rx ring sk_buffs */
+	/* Free all the Rx ring frags */
 	for (i = 0; i < rx_ring->count; i++) {
 		struct igb_rx_buffer *buffer_info = &rx_ring->rx_buffer_info[i];
 		if (buffer_info->dma) {
@@ -3390,9 +3390,9 @@ static void igb_clean_rx_ring(struct igb_ring *rx_ring)
 			buffer_info->dma = 0;
 		}
 
-		if (buffer_info->skb) {
-			dev_kfree_skb(buffer_info->skb);
-			buffer_info->skb = NULL;
+		if (buffer_info->data) {
+			put_page(virt_to_head_page(buffer_info->data));
+			buffer_info->data = NULL;
 		}
 		if (buffer_info->page_dma) {
 			dma_unmap_page(rx_ring->dev,
@@ -3407,6 +3407,8 @@ static void igb_clean_rx_ring(struct igb_ring *rx_ring)
 			buffer_info->page_offset = 0;
 		}
 	}
+	kfree_skb(rx_ring->skb);
+	rx_ring->skb = NULL;
 
 	size = sizeof(struct igb_rx_buffer) * rx_ring->count;
 	memset(rx_ring->rx_buffer_info, 0, size);
@@ -6078,11 +6080,13 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget)
 
 	while (igb_test_staterr(rx_desc, E1000_RXD_STAT_DD)) {
 		struct igb_rx_buffer *buffer_info = &rx_ring->rx_buffer_info[i];
-		struct sk_buff *skb = buffer_info->skb;
+		struct sk_buff *skb = rx_ring->skb;
+		void *data = buffer_info->data;
 		union e1000_adv_rx_desc *next_rxd;
 
-		buffer_info->skb = NULL;
-		prefetch(skb->data);
+		prefetch(data + NET_SKB_PAD);
+		buffer_info->data = NULL;
+		rx_ring->skb = NULL;
 
 		i++;
 		if (i == rx_ring->count)
@@ -6091,6 +6095,15 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget)
 		next_rxd = IGB_RX_DESC(rx_ring, i);
 		prefetch(next_rxd);
 
+		if (!skb) {
+			skb = build_skb(data, IGB_FRAGSZ);
+			if (unlikely(!skb)) {
+				rx_ring->rx_stats.alloc_failed++;
+				buffer_info->data = data;
+				goto next_desc;
+			}
+			skb_reserve(skb, NET_SKB_PAD + NET_IP_ALIGN);
+		}
 		/*
 		 * This memory barrier is needed to keep us from reading
 		 * any other fields out of the rx_desc until we know the
@@ -6118,6 +6131,8 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget)
 			skb->data_len += length;
 			skb->truesize += PAGE_SIZE / 2;
 
+			skb_propagate_pfmemalloc(buffer_info->page, skb);
+
 			if ((page_count(buffer_info->page) != 1) ||
 			    (page_to_nid(buffer_info->page) != current_node))
 				buffer_info->page = NULL;
@@ -6132,9 +6147,9 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget)
 		if (!igb_test_staterr(rx_desc, E1000_RXD_STAT_EOP)) {
 			struct igb_rx_buffer *next_buffer;
 			next_buffer = &rx_ring->rx_buffer_info[i];
-			buffer_info->skb = next_buffer->skb;
+			buffer_info->data = next_buffer->data;
 			buffer_info->dma = next_buffer->dma;
-			next_buffer->skb = skb;
+			rx_ring->skb = skb;
 			next_buffer->dma = 0;
 			goto next_desc;
 		}
@@ -6158,6 +6173,7 @@ static bool igb_clean_rx_irq(struct igb_q_vector *q_vector, int budget)
 
 		skb->protocol = eth_type_trans(skb, rx_ring->netdev);
 
+		skb_record_rx_queue(skb, rx_ring->queue_index);
 		napi_gro_receive(&q_vector->napi, skb);
 
 		budget--;
@@ -6193,26 +6209,22 @@ next_desc:
 static bool igb_alloc_mapped_skb(struct igb_ring *rx_ring,
 				 struct igb_rx_buffer *bi)
 {
-	struct sk_buff *skb = bi->skb;
+	void *data = bi->data;
 	dma_addr_t dma = bi->dma;
 
 	if (dma)
 		return true;
 
-	if (likely(!skb)) {
-		skb = netdev_alloc_skb_ip_align(rx_ring->netdev,
-						IGB_RX_HDR_LEN);
-		bi->skb = skb;
-		if (!skb) {
+	if (likely(!data)) {
+		data = netdev_alloc_frag(IGB_FRAGSZ);
+		bi->data = data;
+		if (!data) {
 			rx_ring->rx_stats.alloc_failed++;
 			return false;
 		}
-
-		/* initialize skb for ring */
-		skb_record_rx_queue(skb, rx_ring->queue_index);
 	}
 
-	dma = dma_map_single(rx_ring->dev, skb->data,
+	dma = dma_map_single(rx_ring->dev, data + NET_SKB_PAD + NET_IP_ALIGN,
 			     IGB_RX_HDR_LEN, DMA_FROM_DEVICE);
 
 	if (dma_mapping_error(rx_ring->dev, dma)) {
@@ -6235,7 +6247,7 @@ static bool igb_alloc_mapped_page(struct igb_ring *rx_ring,
 		return true;
 
 	if (!page) {
-		page = __skb_alloc_page(GFP_ATOMIC, bi->skb);
+		page = alloc_page(GFP_ATOMIC | __GFP_COLD);
 		bi->page = page;
 		if (unlikely(!page)) {
 			rx_ring->rx_stats.alloc_failed++;

^ permalink raw reply related

* Re: [RFC PATCH 01/10] net: Split core bits of dev_pick_tx into __dev_pick_tx
From: Alexander Duyck @ 2012-08-02 15:45 UTC (permalink / raw)
  To: Ying Cai
  Cc: Ben Hutchings, netdev, davem, jeffrey.t.kirsher, edumazet,
	therbert, alexander.duyck
In-Reply-To: <CAL1qit_mpmVYQ3D4HQsii5LJ+Nu5=ftFWAWVnfPiDbmW5eWa0Q@mail.gmail.com>

On 08/01/2012 08:51 PM, Ying Cai wrote:
> I liked this patch too. It enabled Ethernet NIC drivers to use
> the __dev_pick_tx() in their ndo_select_queue() to use XPS. I have
> data showing XPS helps performances of bnx2x driver significantly.
>
> Acked-by: Ying Cai <ycai@ <mailto:jg1.han@samsung.com>google.com
> <http://google.com>>
This was an RFC so there is no need to ack it.

I will try to get these patches cleaned up and ready for submission. 
Hopefully I will have something to submit to net-next in the next couple
of weeks.

Thanks,

Alex

^ permalink raw reply

* Re: [PATCH] igb: reduce Rx header size
From: Alexander Duyck @ 2012-08-02 15:43 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Jeff Kirsher, netdev, Wyborny, Carolyn
In-Reply-To: <1343920538.9299.200.camel@edumazet-glaptop>

On 08/02/2012 08:15 AM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Reduce skb truesize by 256 bytes.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> Tested on my machine without any problem
>
>  drivers/net/ethernet/intel/igb/igb.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 9e572dd..0c9f62c 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -131,9 +131,9 @@ struct vf_data_storage {
>  #define MAXIMUM_ETHERNET_VLAN_SIZE 1522
>  
>  /* Supported Rx Buffer Sizes */
> -#define IGB_RXBUFFER_512   512
> +#define IGB_RXBUFFER_256   256
>  #define IGB_RXBUFFER_16384 16384
> -#define IGB_RX_HDR_LEN     IGB_RXBUFFER_512
> +#define IGB_RX_HDR_LEN     IGB_RXBUFFER_256
>  
>  /* How many Tx Descriptors do we need to call netif_wake_queue ? */
>  #define IGB_TX_QUEUE_WAKE	16
>
>
Would there be any problems with us running this through Jeff's tree for
testing prior to applying this?  I'm just wanting to do some testing
internally for any performance regressions due to this change.

Thanks,

Alex

^ permalink raw reply

* Re: [PATCH V2 08/12] net/eipoib: Add ethtool file support
From: Ben Hutchings @ 2012-08-02 15:42 UTC (permalink / raw)
  To: Erez Shitrit
  Cc: Or Gerlitz, davem, roland, netdev, ali, sean.hefty, Erez Shitrit
In-Reply-To: <501A3BDE.1070509@mellanox.com>

On Thu, 2012-08-02 at 11:35 +0300, Erez Shitrit wrote:
> On 8/2/2012 3:22 AM, Ben Hutchings wrote:
> > On Wed, 2012-08-01 at 20:09 +0300, Or Gerlitz wrote:
> >> From: Erez Shitrit <erezsh@mellanox.co.il>
> >>
> >> Via ethtool the driver describes its version, ABI version, on what PIF
> >> interface it runs and various statistics.
> > [...]
> >> --- /dev/null
> >> +++ b/drivers/net/eipoib/eth_ipoib_ethtool.c
> > [...]
> >> +static void parent_ethtool_get_drvinfo(struct net_device *parent_dev,
> >> +				       struct ethtool_drvinfo *drvinfo)
> >> +{
> >> +	struct parent *parent = netdev_priv(parent_dev);
> >> +
> >> +	strncpy(drvinfo->driver, DRV_NAME, 32);
> >> +
> >> +	strncpy(drvinfo->version, DRV_VERSION, 32);
> >> +
> >> +	strncpy(drvinfo->bus_info, parent->ipoib_main_interface,
> >> +		ETHTOOL_BUSINFO_LEN);
> > These must be null-terminated; therefore use strlcpy().
> 
> ok, will fix.
> 
> >> +	/* indicates ABI version */
> >> +	snprintf(drvinfo->fw_version, 32, "%d", EIPOIB_ABI_VER);
> > [...]
> >
> > This is an abuse of fw_version.
> >
> > Ben.
> we took the idea from the bonding driver,
> (snprintf(drvinfo->fw_version, 32, "%d", BOND_ABI_VERSION);)

The bonding driver has lots of warts.

> Do you have any idea where can we keep the abi version?

You don't need to, because David will insist that you will only change
the ABI in a backward-compatible way. :-)

(The bonding ABI version hasn't changed since Linux 2.6.3, and even then
it provided backward compatibility.)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply

* [PATCH 0/4] net/mlx4: Fixes to mlx4 driver
From: Yevgeny Petrilin @ 2012-08-02 15:30 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yevgeny Petrilin

Hello Dave,

This is a patchset of 3 fixes and additional change that removes
a port Link layer type restriction that is no longer relevant.

Thanks,
Yevgeny
---
Yevgeny Petrilin (3):
  net/mlx4_en: Setting the NETIF_F_GRO flag back to dev->hw_features
  net/mlx4_en: Fixing TX queue stop/wake flow
  net/mlx4_core: Remove port type restrictions

Amir Vadai (1):
  net/mlx4_en: loopbacked packets are dropped when SMAC=DMAC

 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    3 ++-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c     |    4 ++--
 drivers/net/ethernet/mellanox/mlx4/en_tx.c     |   17 +++++++----------
 drivers/net/ethernet/mellanox/mlx4/main.c      |    3 ---
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h   |    1 -
 drivers/net/ethernet/mellanox/mlx4/sense.c     |   14 --------------
 6 files changed, 11 insertions(+), 31 deletions(-)

^ permalink raw reply

* [PATCH 4/4] net/mlx4_core: Remove port type restrictions
From: Yevgeny Petrilin @ 2012-08-02 15:30 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yevgeny Petrilin
In-Reply-To: <1343921456-19180-1-git-send-email-yevgenyp@mellanox.co.il>

Port1=Eth, Port2=IB restriction is no longer required.
Having RoCE, there will always rdma port initialized over ConnectX
physical port, no matter whether the link layer is IB or Ethernet.
So we always have dual port IB device.

Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
---
 drivers/net/ethernet/mellanox/mlx4/main.c  |    3 ---
 drivers/net/ethernet/mellanox/mlx4/sense.c |   14 --------------
 2 files changed, 0 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 48d0e90..827b72d 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -157,9 +157,6 @@ int mlx4_check_port_params(struct mlx4_dev *dev,
 					 "on this HCA, aborting.\n");
 				return -EINVAL;
 			}
-			if (port_type[i] == MLX4_PORT_TYPE_ETH &&
-			    port_type[i + 1] == MLX4_PORT_TYPE_IB)
-				return -EINVAL;
 		}
 	}
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/sense.c b/drivers/net/ethernet/mellanox/mlx4/sense.c
index 8024982..34ee09b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/sense.c
+++ b/drivers/net/ethernet/mellanox/mlx4/sense.c
@@ -81,20 +81,6 @@ void mlx4_do_sense_ports(struct mlx4_dev *dev,
 	}
 
 	/*
-	 * Adjust port configuration:
-	 * If port 1 sensed nothing and port 2 is IB, set both as IB
-	 * If port 2 sensed nothing and port 1 is Eth, set both as Eth
-	 */
-	if (stype[0] == MLX4_PORT_TYPE_ETH) {
-		for (i = 1; i < dev->caps.num_ports; i++)
-			stype[i] = stype[i] ? stype[i] : MLX4_PORT_TYPE_ETH;
-	}
-	if (stype[dev->caps.num_ports - 1] == MLX4_PORT_TYPE_IB) {
-		for (i = 0; i < dev->caps.num_ports - 1; i++)
-			stype[i] = stype[i] ? stype[i] : MLX4_PORT_TYPE_IB;
-	}
-
-	/*
 	 * If sensed nothing, remain in current configuration.
 	 */
 	for (i = 0; i < dev->caps.num_ports; i++)
-- 
1.7.7

^ permalink raw reply related

* [PATCH 1/4] net/mlx4_en: Setting the NETIF_F_GRO flag back to dev->hw_features
From: Yevgeny Petrilin @ 2012-08-02 15:30 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yevgeny Petrilin
In-Reply-To: <1343921456-19180-1-git-send-email-yevgenyp@mellanox.co.il>

The bug which removed it was introduced in commit c8c64cff
which added the hw_features.

Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index edd9cb8..c031e12 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -1658,7 +1658,8 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
 	/*
 	 * Set driver features
 	 */
-	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
+			   NETIF_F_GRO;
 	if (mdev->LSO_support)
 		dev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
 
-- 
1.7.7

^ permalink raw reply related

* [PATCH 2/4] net/mlx4_en: loopbacked packets are dropped when SMAC=DMAC
From: Yevgeny Petrilin @ 2012-08-02 15:30 UTC (permalink / raw)
  To: davem; +Cc: netdev, Amir Vadai
In-Reply-To: <1343921456-19180-1-git-send-email-yevgenyp@mellanox.co.il>

From: Amir Vadai <amirv@mellanox.com>

Should NOT check SMAC=DMAC when:
1. loopback is turned on
2. validate_loopback is true.

Fixed it accordingly.

Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index f32e703..5aba5ec 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -614,8 +614,8 @@ int mlx4_en_process_rx_cq(struct net_device *dev, struct mlx4_en_cq *cq, int bud
 		/* If source MAC is equal to our own MAC and not performing
 		 * the selftest or flb disabled - drop the packet */
 		if (s_mac == priv->mac &&
-			(!(dev->features & NETIF_F_LOOPBACK) ||
-			 !priv->validate_loopback))
+		    !((dev->features & NETIF_F_LOOPBACK) ||
+		      priv->validate_loopback))
 			goto next;
 
 		/*
-- 
1.7.7

^ permalink raw reply related

* [PATCH 3/4] net/mlx4_en: Fixing TX queue stop/wake flow
From: Yevgeny Petrilin @ 2012-08-02 15:30 UTC (permalink / raw)
  To: davem; +Cc: netdev, Yevgeny Petrilin
In-Reply-To: <1343921456-19180-1-git-send-email-yevgenyp@mellanox.co.il>

Removing the ring->blocked flag, it is redundant and leads to a race:

We close the TX queue and then set the "blocked" flag.
Between those 2 operations the completion function can check the "blocked"
flag, sees that it is 0, and wouldn't open the TX queue.

Using netif_tx_queue_stopped to check the state of the queue to avoid this race.

Signed-off-by: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c   |   17 +++++++----------
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h |    1 -
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 019d856..10bba09 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -164,7 +164,6 @@ int mlx4_en_activate_tx_ring(struct mlx4_en_priv *priv,
 	ring->cons = 0xffffffff;
 	ring->last_nr_txbb = 1;
 	ring->poll_cnt = 0;
-	ring->blocked = 0;
 	memset(ring->tx_info, 0, ring->size * sizeof(struct mlx4_en_tx_info));
 	memset(ring->buf, 0, ring->buf_size);
 
@@ -365,14 +364,13 @@ static void mlx4_en_process_tx_cq(struct net_device *dev, struct mlx4_en_cq *cq)
 	ring->cons += txbbs_skipped;
 	netdev_tx_completed_queue(ring->tx_queue, packets, bytes);
 
-	/* Wakeup Tx queue if this ring stopped it */
-	if (unlikely(ring->blocked)) {
-		if ((u32) (ring->prod - ring->cons) <=
-		     ring->size - HEADROOM - MAX_DESC_TXBBS) {
-			ring->blocked = 0;
-			netif_tx_wake_queue(ring->tx_queue);
-			priv->port_stats.wake_queue++;
-		}
+	/*
+	 * Wakeup Tx queue if this stopped, and at least 1 packet
+	 * was completed
+	 */
+	if (netif_tx_queue_stopped(ring->tx_queue) && txbbs_skipped > 0) {
+		netif_tx_wake_queue(ring->tx_queue);
+		priv->port_stats.wake_queue++;
 	}
 }
 
@@ -592,7 +590,6 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		     ring->size - HEADROOM - MAX_DESC_TXBBS)) {
 		/* every full Tx ring stops queue */
 		netif_tx_stop_queue(ring->tx_queue);
-		ring->blocked = 1;
 		priv->port_stats.queue_stopped++;
 
 		return NETDEV_TX_BUSY;
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index 5f1ab10..9d27e42 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -248,7 +248,6 @@ struct mlx4_en_tx_ring {
 	u32 doorbell_qpn;
 	void *buf;
 	u16 poll_cnt;
-	int blocked;
 	struct mlx4_en_tx_info *tx_info;
 	u8 *bounce_buf;
 	u32 last_nr_txbb;
-- 
1.7.7

^ permalink raw reply related

* Re: [PATCH V2 06/12] net/core: Add rtnetlink support to vif parameters
From: Erez Shitrit @ 2012-08-02 15:29 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Or Gerlitz, davem, roland, netdev, ali, sean.hefty, Erez Shitrit
In-Reply-To: <1343866837.2602.64.camel@bwh-desktop.uk.solarflarecom.com>

On 8/2/2012 3:20 AM, Ben Hutchings wrote:
> On Wed, 2012-08-01 at 20:09 +0300, Or Gerlitz wrote:
>> From: Erez Shitrit <erezsh@mellanox.co.il>
>>
>> Add IFLA_VIF_INFO which allows user space to configure mac/vlan for a VIF
> [...]
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -780,7 +780,8 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
>>   	       + rtnl_vfinfo_size(dev, ext_filter_mask) /* IFLA_VFINFO_LIST */
>>   	       + rtnl_port_size(dev) /* IFLA_VF_PORTS + IFLA_PORT_SELF */
>>   	       + rtnl_link_get_size(dev) /* IFLA_LINKINFO */
>> -	       + rtnl_link_get_af_size(dev); /* IFLA_AF_SPEC */
>> +	       + rtnl_link_get_af_size(dev) /* IFLA_AF_SPEC */
>> +	       + nla_total_size(sizeof(struct ifla_vif_param)); /* IFLA_VIF_INFO */
>>   }
> [...]
>
> if_nlmsg_size() returns the size of a message describing the interface.
> But IFLA_VIF_INFO is write-only (why?) and therefore shouldn't be
> included.
>
> Ben.

thanks, we missed that.
currently, we don't expose the vifs on its parent interface, we need to 
think if we want to expose it, if yes we will add "get" option, 
otherwise we will take it from if_nlmsg_size() function.

^ permalink raw reply

* [PATCH] igb: reduce Rx header size
From: Eric Dumazet @ 2012-08-02 15:15 UTC (permalink / raw)
  To: Jeff Kirsher; +Cc: Alexander Duyck, netdev
In-Reply-To: <1342912142-11130-8-git-send-email-jeffrey.t.kirsher@intel.com>

From: Eric Dumazet <edumazet@google.com>

Reduce skb truesize by 256 bytes.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
---
Tested on my machine without any problem

 drivers/net/ethernet/intel/igb/igb.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 9e572dd..0c9f62c 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -131,9 +131,9 @@ struct vf_data_storage {
 #define MAXIMUM_ETHERNET_VLAN_SIZE 1522
 
 /* Supported Rx Buffer Sizes */
-#define IGB_RXBUFFER_512   512
+#define IGB_RXBUFFER_256   256
 #define IGB_RXBUFFER_16384 16384
-#define IGB_RX_HDR_LEN     IGB_RXBUFFER_512
+#define IGB_RX_HDR_LEN     IGB_RXBUFFER_256
 
 /* How many Tx Descriptors do we need to call netif_wake_queue ? */
 #define IGB_TX_QUEUE_WAKE	16

^ permalink raw reply related

* Huge performance degradation for UDP between kernel 2.4.17 and 2.6.x
From: leroy christophe @ 2012-08-02 14:02 UTC (permalink / raw)
  To: netdev

Hi

I'm having a big issue with UDP. Using a powerpc board (MPC860).

With our board running kernel 2.4.17, I'm able to send 160000 voice 
packets (UDP, 96 bytes per packet) in 11 seconds.
With the same board running either Kernel 2.6.35.14 or Kernel 3.4.7, I 
need 55 seconds to send the same amount of packets.


Is there anything to tune in order to get same output rate as with 
Kernel 2.4 ?

Regards
Christophe

^ permalink raw reply


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