* IPsec+SCTP+IPv6 bug (was: Re: https://bugzilla.kernel.org/show_bug.cgi?id=24412)
From: Alexey Dobriyan @ 2013-09-06 16:56 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev
In-Reply-To: <5229B37A.2070900@redhat.com>
[add netdev@ to CC]
My notes and recollections are below.
Bug reporter says traffic is unecnrypted which is technically a different thing,
but when I tried to reproduce absense of encryption, I couldn't even get past
established TCP connection.
IPv6 case works (worked) without IPsec (setkey -F; setkey -FP).
IPv4 case worked with IPsec.
IPv6 didn't work with IPsec.
setkey(8) setup:
#!/usr/sbin/setkey -f
flush;
spdflush;
add A B ah 0x42 -A hmac-sha256 0xKEY1;
add B A ah 0x43 -A hmac-sha256 0xKEY2;
add A B esp 0x44 -E blowfish-cbc 0xKEY3;
add B A esp 0x45 -E blowfish-cbc 0xKEY4;
spdadd A B any -P in ipsec esp/transport//require ah/transport//require;
spdadd B A any -P in ipsec esp/transport//require ah/transport//require;
A, B -- IPv4 or IPv6 client/server addresses.
Client and server copy of the file should have matching keys and SPI numbers,
but "opposite" addresses (A <=> B).
Keys are written in hex.
IIRC setkey is picky about key lengths (they have to match exactly those of crypto algorithms).
--------------------------------------------------------------------
IPv4 reproducer:
socat sctp-listen:3333 -
echo plaintext | socat - sctp-connect:IP4IP4IP4IP4:3333
IPv6 reproducer:
socat sctp6-listen:3333 -
#include <sys/socket.h>
#include <arpa/inet.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <unistd.h>
#include <string.h>
int main(void)
{
struct sockaddr_in6 sa6 = {};
int fd;
fd = socket(PF_INET6, SOCK_STREAM, 0x84);
sa6.sin6_family = AF_INET6;
sa6.sin6_port = htons(3333);
inet_pton(AF_INET6, "IP6IP6IP6IP6IP6IP6", &sa6.sin6_addr);
sa6.sin6_scope_id = 2; /* it depends */
connect(fd, (struct sockaddr *)&sa6, sizeof(struct sockaddr_in6));
write(fd, "plaintext\n", strlen("plaintext\n"));
close(fd);
return 0;
}
^ permalink raw reply
* Re: GSO/GRO and UDP performance
From: Rick Jones @ 2013-09-06 16:42 UTC (permalink / raw)
To: James Yonan; +Cc: Eric Dumazet, netdev
In-Reply-To: <1378472829.31445.21.camel@edumazet-glaptop>
On 09/06/2013 06:07 AM, Eric Dumazet wrote:
> On Fri, 2013-09-06 at 03:22 -0600, James Yonan wrote:
>
>> So I think that playing well with GSO/GRO is essential to get speedup in
>> UDP apps because of this 43x multiplier.
>>
>
> Thats not true. GRO cannot aggregate more than 16+1 packets.
>
> I think we cannot aggregate UDP packets, because UDP lacks sequence
> numbers, so reorders would be a problem.
>
> You really need something that is not UDP generic.
It may not be as sexy, and it cannot get the 43x multiplier (just what
*is* the service demand change on a netperf TCP_STREAM test these days
between GSO/GRO on and off anyway?), but looking for basic path-length
reductions would be goodness.
rick jones
^ permalink raw reply
* Re: TSQ accounting skb->truesize degrades throughput for large packets
From: Zoltan Kiss @ 2013-09-06 16:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wei Liu, Jonathan Davies, Ian Campbell, netdev, xen-devel
In-Reply-To: <1378472268.31445.15.camel@edumazet-glaptop>
On 06/09/13 13:57, Eric Dumazet wrote:
> Well, I have no problem to get line rate on 20Gb with a single flow, so
> other drivers have no problem.
I've made some tests on bare metal:
Dell PE R815, Intel 82599EB 10Gb, 3.11-rc4 32 bit kernel with 3.17.3
ixgbe (TSO, GSO on), iperf 2.0.5
Transmitting packets toward the remote end (so running iperf -c on this
host) can make 8.3 Gbps with the default 128k tcp_limit_output_bytes.
When I increased this to 131.506 (128k + 434 bytes) suddenly it jumped
to 9.4 Gbps. Iperf CPU usage also jumped a few percent from ~36 to ~40%
(softint percentage in top also increased from ~3 to ~5%)
So I guess it would be good to revisit the default value of this
setting. What hw you used Eric for your 20Gb results?
Regards,
Zoli
^ permalink raw reply
* Re: [PATCH net] bnx2x: fix broken compilation with CONFIG_BNX2X_SRIOV is not set
From: Eric Dumazet @ 2013-09-06 16:10 UTC (permalink / raw)
To: Dmitry Kravkov; +Cc: netdev, davem, Ariel Elior
In-Reply-To: <1378452928-6072-1-git-send-email-dmitry@broadcom.com>
On Fri, 2013-09-06 at 10:35 +0300, Dmitry Kravkov wrote:
> Since commit 60cad4e67bd6ff400e7ea61fe762b3042b12ae9d
> "bnx2x: VF RSS support - VF side" fails to compile w/o
> CONFIG_BNX2X_SRIOV option.
>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Ariel Elior <ariele@broadcom.com>
> Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> ---
Thanks !
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net-next v2] net: add documentation for BQL helpers
From: Eric Dumazet @ 2013-09-06 16:09 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, davem
In-Reply-To: <1378483080-31632-1-git-send-email-f.fainelli@gmail.com>
On Fri, 2013-09-06 at 16:58 +0100, Florian Fainelli wrote:
> Provide a kernel-doc comment documentation for the BQL helpers:
> - netdev_sent_queue
> - netdev_completed_queue
> - netdev_reset_queue
>
> Similarly to how it is done for the other functions, the documentation
> only covers the function operating on struct net_device and not struct
> netdev_queue.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Change since v1:
> - incorporate suggestsions from Eric Dumazet and just specify the exact
> match for netdev_sent_queue/netdev_completed_queue
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks
^ permalink raw reply
* [PATCH net-next v2] net: add documentation for BQL helpers
From: Florian Fainelli @ 2013-09-06 15:58 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet, davem, Florian Fainelli
In-Reply-To: <1378465119-1894-1-git-send-email-f.fainelli@gmail.com>
Provide a kernel-doc comment documentation for the BQL helpers:
- netdev_sent_queue
- netdev_completed_queue
- netdev_reset_queue
Similarly to how it is done for the other functions, the documentation
only covers the function operating on struct net_device and not struct
netdev_queue.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Change since v1:
- incorporate suggestsions from Eric Dumazet and just specify the exact
match for netdev_sent_queue/netdev_completed_queue
include/linux/netdevice.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8ed4ae9..041b42a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2101,6 +2101,15 @@ static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
#endif
}
+/**
+ * netdev_sent_queue - report the number of bytes queued to hardware
+ * @dev: network device
+ * @bytes: number of bytes queued to the hardware device queue
+ *
+ * Report the number of bytes queued for sending/completion to the network
+ * device hardware queue. @bytes should be a good approximation and should
+ * exactly match netdev_completed_queue() @bytes
+ */
static inline void netdev_sent_queue(struct net_device *dev, unsigned int bytes)
{
netdev_tx_sent_queue(netdev_get_tx_queue(dev, 0), bytes);
@@ -2130,6 +2139,16 @@ static inline void netdev_tx_completed_queue(struct netdev_queue *dev_queue,
#endif
}
+/**
+ * netdev_completed_queue - report bytes and packets completed by device
+ * @dev: network device
+ * @pkts: actual number of packets sent over the medium
+ * @bytes: actual number of bytes sent over the medium
+ *
+ * Report the number of bytes and packets transmitted by the network device
+ * hardware queue over the physical medium, @bytes must exactly match the
+ * @bytes amount passed to netdev_sent_queue()
+ */
static inline void netdev_completed_queue(struct net_device *dev,
unsigned int pkts, unsigned int bytes)
{
@@ -2144,6 +2163,13 @@ static inline void netdev_tx_reset_queue(struct netdev_queue *q)
#endif
}
+/**
+ * netdev_reset_queue - reset the packets and bytes count of a network device
+ * @dev_queue: network device
+ *
+ * Reset the bytes and packet count of a network device and clear the
+ * software flow control OFF bit for this network device
+ */
static inline void netdev_reset_queue(struct net_device *dev_queue)
{
netdev_tx_reset_queue(netdev_get_tx_queue(dev_queue, 0));
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH net-next] net: add documentation for BQL helpers
From: Eric Dumazet @ 2013-09-06 15:18 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, davem
In-Reply-To: <1378465119-1894-1-git-send-email-f.fainelli@gmail.com>
On Fri, 2013-09-06 at 11:58 +0100, Florian Fainelli wrote:
>
> +/**
> + * netdev_completed_queue - report bytes and packets completed by device
> + * @dev: network device
> + * @pkts: actual number of packets sent over the medium
> + * @bytes: actual number of bytes sent over the medium
> + *
> + * Report the number of bytes and packets transmitted by the network device
> + * hardware queue over the physical medium (without prepended/appended
> + * control blocks, FCS...)
Here, you should instead document that the @bytes must exactly match the
amount given in netdev_sent_queue(), or else BQL can panic.
(each queued skb accounts for X bytes on BQL, so at TX completion, same
X must be unaccounted)
^ permalink raw reply
* [RFC PATCHv2 4/4] net: mvneta: add support for fixed links
From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw)
To: David S. Miller, netdev, devicetree
Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia,
linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner
In-Reply-To: <1378480701-12908-1-git-send-email-thomas.petazzoni@free-electrons.com>
Following the introduction of of_phy_register_fixed_link(), this patch
introduces fixed link support in the mvneta driver, for Marvell Armada
370/XP SOCs.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
.../devicetree/bindings/net/marvell-armada-370-neta.txt | 4 ++--
drivers/net/ethernet/marvell/mvneta.c | 10 ++++++----
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
index 859a6fa..4d07d4e 100644
--- a/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
+++ b/Documentation/devicetree/bindings/net/marvell-armada-370-neta.txt
@@ -4,8 +4,8 @@ Required properties:
- compatible: should be "marvell,armada-370-neta".
- reg: address and length of the register set for the device.
- interrupts: interrupt for the device
-- phy: A phandle to a phy node defining the PHY address (as the reg
- property, a single integer).
+- phy: A phandle to the PHY node describing the PHY to which this
+ Ethernet controller is connected to.
- phy-mode: The interface between the SoC and the PHY (a string that
of_get_phy_mode() can understand)
- clocks: a pointer to the reference clock for this device.
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b017818..6da1516 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2711,10 +2711,12 @@ static int mvneta_probe(struct platform_device *pdev)
}
phy_node = of_parse_phandle(dn, "phy", 0);
- if (!phy_node) {
- dev_err(&pdev->dev, "no associated PHY\n");
- err = -ENODEV;
- goto err_free_irq;
+ if (of_phy_is_fixed_link(phy_node)) {
+ err = of_phy_register_fixed_link(phy_node);
+ if (err < 0) {
+ dev_err(&pdev->dev, "cannot register fixed PHY\n");
+ goto err_free_irq;
+ }
}
phy_mode = of_get_phy_mode(dn);
--
1.8.1.2
^ permalink raw reply related
* [RFC PATCHv2 3/4] of: provide a binding for fixed link PHYs
From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw)
To: David S. Miller, netdev, devicetree
Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia,
linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner
In-Reply-To: <1378480701-12908-1-git-send-email-thomas.petazzoni@free-electrons.com>
Some Ethernet MACs have a "fixed link", and are not connected to a
normal MDIO-managed PHY device. For those situations, a Device Tree
binding allows to describe a "fixed link" using a special PHY node.
This patch adds:
* A documentation for the fixed PHY Device Tree binding.
* An of_phy_is_fixed_link() function that an Ethernet driver can call
on its PHY phandle to find out whether it's a fixed link PHY or
not. It should typically be used to know if
of_phy_register_fixed_link() should be called.
* An of_phy_register_fixed_link() function that instantiates the
fixed PHY into the PHY subsystem, so that when the driver calls
of_phy_connect(), the PHY device associated to the OF node will be
found.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
.../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++
drivers/of/of_mdio.c | 24 +++++++++++++++
include/linux/of_mdio.h | 15 ++++++++++
3 files changed, 73 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt
diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
new file mode 100644
index 0000000..9f2a1a50
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/fixed-link.txt
@@ -0,0 +1,34 @@
+Fixed link Device Tree binding
+------------------------------
+
+Some Ethernet MACs have a "fixed link", and are not connected to a
+normal MDIO-managed PHY device. For those situations, a Device Tree
+binding allows to describe a "fixed link".
+
+Such a fixed link situation is described by creating a PHY node as a
+sub-node of an Ethernet device, with the following properties:
+
+* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a
+ fixed link PHY.
+* 'speed' (integer, mandatory), to indicate the link speed. Accepted
+ values are 10, 100 and 1000
+* 'full-duplex' (boolean, optional), to indicate that full duplex is
+ used. When absent, half duplex is assumed.
+* 'pause' (boolean, optional), to indicate that pause should be
+ enabled.
+* 'asym-pause' (boolean, optional), to indicate that asym_pause should
+ be enabled.
+
+Example:
+
+ethernet@0 {
+ ...
+ phy = <&phy0>;
+ phy0: phy@0 {
+ fixed-link;
+ speed = <1000>;
+ full-duplex;
+ };
+ ...
+};
+
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index d5a57a9..0507f8f 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -14,6 +14,7 @@
#include <linux/netdevice.h>
#include <linux/err.h>
#include <linux/phy.h>
+#include <linux/phy_fixed.h>
#include <linux/of.h>
#include <linux/of_irq.h>
#include <linux/of_mdio.h>
@@ -247,3 +248,26 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
return IS_ERR(phy) ? NULL : phy;
}
EXPORT_SYMBOL(of_phy_connect_fixed_link);
+
+#if defined(CONFIG_FIXED_PHY)
+bool of_phy_is_fixed_link(struct device_node *np)
+{
+ return of_property_read_bool(np, "fixed-link");
+}
+EXPORT_SYMBOL(of_phy_is_fixed_link);
+
+int of_phy_register_fixed_link(struct device_node *np)
+{
+ struct fixed_phy_status status = {};
+
+ status.link = 1;
+ status.duplex = of_property_read_bool(np, "full-duplex");
+ if (of_property_read_u32(np, "speed", &status.speed))
+ return -EINVAL;
+ status.pause = of_property_read_bool(np, "pause");
+ status.asym_pause = of_property_read_bool(np, "asym-pause");
+
+ return fixed_phy_register(PHY_POLL, &status, np);
+}
+EXPORT_SYMBOL(of_phy_register_fixed_link);
+#endif
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 8163107..2f535ee 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -57,4 +57,19 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)
}
#endif /* CONFIG_OF */
+#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
+extern int of_phy_register_fixed_link(struct device_node *np);
+extern bool of_phy_is_fixed_link(struct device_node *np);
+#else
+static inline int of_phy_register_fixed_link(struct device_node *np)
+{
+ return -ENOSYS;
+}
+static inline bool of_phy_is_fixed_link(struct device_node *np)
+{
+ return false;
+}
+#endif
+
+
#endif /* __LINUX_OF_MDIO_H */
--
1.8.1.2
^ permalink raw reply related
* [RFC PATCHv2 2/4] net: phy: extend fixed driver with fixed_phy_register()
From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw)
To: David S. Miller, netdev, devicetree
Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia,
linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner
In-Reply-To: <1378480701-12908-1-git-send-email-thomas.petazzoni@free-electrons.com>
The existing fixed_phy_add() function has several drawbacks that
prevents it from being used as is for OF-based declaration of fixed
PHYs:
* The address of the PHY on the fake bus needs to be passed, while a
dynamic allocation is desired.
* Since the phy_device instantiation is post-poned until the next
mdiobus scan, there is no way to associate the fixed PHY with its
OF node, which later prevents of_phy_connect() from finding this
fixed PHY from a given OF node.
To solve this, this commit introduces fixed_phy_register(), which will
allocate an available PHY address, add the PHY using fixed_phy_add()
and instantiate the phy_device structure associated with the provided
OF node.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
drivers/net/phy/fixed.c | 43 +++++++++++++++++++++++++++++++++++++++++++
include/linux/phy_fixed.h | 11 +++++++++++
2 files changed, 54 insertions(+)
diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
index 0f02403..3f9412b 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c
@@ -21,6 +21,7 @@
#include <linux/phy_fixed.h>
#include <linux/err.h>
#include <linux/slab.h>
+#include <linux/of.h>
#define MII_REGS_NUM 29
@@ -203,6 +204,48 @@ err_regs:
}
EXPORT_SYMBOL_GPL(fixed_phy_add);
+static int phy_fixed_addr;
+static DEFINE_SPINLOCK(phy_fixed_addr_lock);
+
+int fixed_phy_register(unsigned int irq,
+ struct fixed_phy_status *status,
+ struct device_node *np)
+{
+ struct fixed_mdio_bus *fmb = &platform_fmb;
+ struct phy_device *phy;
+ int phy_addr;
+ int ret;
+
+ /* Get the next available PHY address, up to PHY_MAX_ADDR */
+ spin_lock(&phy_fixed_addr_lock);
+ if (phy_fixed_addr == PHY_MAX_ADDR) {
+ spin_unlock(&phy_fixed_addr_lock);
+ return -ENOSPC;
+ }
+ phy_addr = phy_fixed_addr++;
+ spin_unlock(&phy_fixed_addr_lock);
+
+ ret = fixed_phy_add(PHY_POLL, phy_addr, status);
+ if (ret < 0)
+ return ret;
+
+ phy = get_phy_device(fmb->mii_bus, phy_addr, false);
+ if (!phy || IS_ERR(phy))
+ return -EINVAL;
+
+ of_node_get(np);
+ phy->dev.of_node = np;
+
+ ret = phy_device_register(phy);
+ if (ret) {
+ phy_device_free(phy);
+ of_node_put(np);
+ return ret;
+ }
+
+ return 0;
+}
+
static int __init fixed_mdio_bus_init(void)
{
struct fixed_mdio_bus *fmb = &platform_fmb;
diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index 509d8f5..902e8a1 100644
--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h
@@ -9,15 +9,26 @@ struct fixed_phy_status {
int asym_pause;
};
+struct device_node;
+
#ifdef CONFIG_FIXED_PHY
extern int fixed_phy_add(unsigned int irq, int phy_id,
struct fixed_phy_status *status);
+extern int fixed_phy_register(unsigned int irq,
+ struct fixed_phy_status *status,
+ struct device_node *np);
#else
static inline int fixed_phy_add(unsigned int irq, int phy_id,
struct fixed_phy_status *status)
{
return -ENODEV;
}
+extern int fixed_phy_register(unsigned int irq,
+ struct fixed_phy_status *status,
+ struct device_node *np)
+{
+ return -ENODEV;
+}
#endif /* CONFIG_FIXED_PHY */
/*
--
1.8.1.2
^ permalink raw reply related
* [RFC PATCHv2 1/4] net: phy: decouple PHY id and PHY address in fixed PHY driver
From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw)
To: David S. Miller, netdev, devicetree
Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia,
linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner
In-Reply-To: <1378480701-12908-1-git-send-email-thomas.petazzoni@free-electrons.com>
Until now, the fixed_phy_add() function was taking as argument
'phy_id', which was used both as the PHY address on the fake fixed
MDIO bus, and as the PHY id, as available in the MII_PHYSID1 and
MII_PHYSID2 registers. However, those two informations are completely
unrelated.
This patch decouples them. The PHY id of fixed PHYs is hardcoded to be
0xdeadbeef. Ideally, a really reserved value would be nicer, but there
doesn't seem to be an easy of making sure a dummy value can be
assigned to the Linux kernel for such usage.
The PHY address remains passed by the caller of phy_fixed_add().
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
drivers/net/phy/fixed.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
index ba55adf..0f02403 100644
--- a/drivers/net/phy/fixed.c
+++ b/drivers/net/phy/fixed.c
@@ -31,7 +31,7 @@ struct fixed_mdio_bus {
};
struct fixed_phy {
- int id;
+ int addr;
u16 regs[MII_REGS_NUM];
struct phy_device *phydev;
struct fixed_phy_status status;
@@ -104,8 +104,8 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
if (fp->status.asym_pause)
lpa |= LPA_PAUSE_ASYM;
- fp->regs[MII_PHYSID1] = fp->id >> 16;
- fp->regs[MII_PHYSID2] = fp->id;
+ fp->regs[MII_PHYSID1] = 0xdead;
+ fp->regs[MII_PHYSID2] = 0xbeef;
fp->regs[MII_BMSR] = bmsr;
fp->regs[MII_BMCR] = bmcr;
@@ -115,7 +115,7 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
return 0;
}
-static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
+static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)
{
struct fixed_mdio_bus *fmb = bus->priv;
struct fixed_phy *fp;
@@ -124,7 +124,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
return -1;
list_for_each_entry(fp, &fmb->phys, node) {
- if (fp->id == phy_id) {
+ if (fp->addr == phy_addr) {
/* Issue callback if user registered it. */
if (fp->link_update) {
fp->link_update(fp->phydev->attached_dev,
@@ -138,7 +138,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_id, int reg_num)
return 0xFFFF;
}
-static int fixed_mdio_write(struct mii_bus *bus, int phy_id, int reg_num,
+static int fixed_mdio_write(struct mii_bus *bus, int phy_addr, int reg_num,
u16 val)
{
return 0;
@@ -160,7 +160,7 @@ int fixed_phy_set_link_update(struct phy_device *phydev,
return -EINVAL;
list_for_each_entry(fp, &fmb->phys, node) {
- if (fp->id == phydev->phy_id) {
+ if (fp->addr == phydev->addr) {
fp->link_update = link_update;
fp->phydev = phydev;
return 0;
@@ -171,7 +171,7 @@ int fixed_phy_set_link_update(struct phy_device *phydev,
}
EXPORT_SYMBOL_GPL(fixed_phy_set_link_update);
-int fixed_phy_add(unsigned int irq, int phy_id,
+int fixed_phy_add(unsigned int irq, int phy_addr,
struct fixed_phy_status *status)
{
int ret;
@@ -184,9 +184,9 @@ int fixed_phy_add(unsigned int irq, int phy_id,
memset(fp->regs, 0xFF, sizeof(fp->regs[0]) * MII_REGS_NUM);
- fmb->irqs[phy_id] = irq;
+ fmb->irqs[phy_addr] = irq;
- fp->id = phy_id;
+ fp->addr = phy_addr;
fp->status = *status;
ret = fixed_phy_update_regs(fp);
--
1.8.1.2
^ permalink raw reply related
* [RFC PATCHv2 0/4] Add DT support for fixed PHYs
From: Thomas Petazzoni @ 2013-09-06 15:18 UTC (permalink / raw)
To: David S. Miller, netdev, devicetree
Cc: Florian Fainelli, Lior Amsalem, Gregory Clement, Ezequiel Garcia,
linux-arm-kernel, Mark Rutland, Sascha Hauer, Christian Gmeiner
Hello,
Here is a second version of the patch set that adds a Device Tree
binding and the related code to support fixed PHYs. Marked as RFC,
this patch set is obviously not intended for merging in 3.12.
Since the first version, the changes have been:
* Instead of using a 'fixed-link' property inside the Ethernet device
DT node, with a fairly cryptic succession of integer values, we now
use a PHY subnode under the Ethernet device DT node, with explicit
properties to configure the duplex, speed, pause and other PHY
properties.
* The PHY address is automatically allocated by the kernel and no
longer visible in the Device Tree binding.
* The PHY device is created directly when the network driver calls
of_phy_connect_fixed_link(), and associated to the PHY DT node,
which allows the existing of_phy_connect() function to work,
without the need to use the deprecated of_phy_connect_fixed_link().
The things I am not entirely happy with yet are:
* The PHY ID is hardcoded to 0xdeadbeef. Ideally, it should be a
properly reserved vendor/device identifier, but it isn't clear how
to get one allocated for this purpose.
* The fixed_phy_register() function in drivers/net/phy/fixed.c has
some OF references. So ideally, I would have preferred to put this
code in drivers/of/of_mdio.c, but to call get_phy_device(), we need
a reference to the mii_bus structure that represents the fixed MDIO
bus.
* There is some error management missing in fixed_phy_register(), but
it can certainly be added easily. This RFC is meant to sort out the
general idea.
Thanks,
Thomas
Thomas Petazzoni (4):
net: phy: decouple PHY id and PHY address in fixed PHY driver
net: phy: extend fixed driver with fixed_phy_register()
of: provide a binding for fixed link PHYs
net: mvneta: add support for fixed links
.../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++
.../bindings/net/marvell-armada-370-neta.txt | 4 +-
drivers/net/ethernet/marvell/mvneta.c | 10 ++--
drivers/net/phy/fixed.c | 63 ++++++++++++++++++----
drivers/of/of_mdio.c | 24 +++++++++
include/linux/of_mdio.h | 15 ++++++
include/linux/phy_fixed.h | 11 ++++
7 files changed, 145 insertions(+), 16 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt
--
1.8.1.2
^ permalink raw reply
* [patch net/stable] ipv6/exthdrs: accept tlv which includes only padding
From: Jiri Pirko @ 2013-09-06 14:02 UTC (permalink / raw)
To: netdev; +Cc: davem, kuznet, jmorris, yoshfuji, kaber, eldad
In rfc4942 and rfc2460 I cannot find anything which would implicate to
drop packets which have only padding in tlv.
Current behaviour breaks TAHI Test v6LC.1.2.6.
Problem was intruduced in:
9b905fe6843 "ipv6/exthdrs: strict Pad1 and PadN check"
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
net/ipv6/exthdrs.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 07a7d65..8d67900 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -162,12 +162,6 @@ static bool ip6_parse_tlv(const struct tlvtype_proc *procs, struct sk_buff *skb)
off += optlen;
len -= optlen;
}
- /* This case will not be caught by above check since its padding
- * length is smaller than 7:
- * 1 byte NH + 1 byte Length + 6 bytes Padding
- */
- if ((padlen == 6) && ((off - skb_network_header_len(skb)) == 8))
- goto bad;
if (len == 0)
return true;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next] net: add documentation for BQL helpers
From: Florian Fainelli @ 2013-09-06 13:56 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David Miller
In-Reply-To: <1378472605.31445.18.camel@edumazet-glaptop>
2013/9/6 Eric Dumazet <eric.dumazet@gmail.com>:
> On Fri, 2013-09-06 at 11:58 +0100, Florian Fainelli wrote:
>> Provide a kernel-doc comment documentation for the BQL helpers:
>> - netdev_sent_queue
>> - netdev_completed_queue
>> - netdev_reset_queue
>>
>> Similarly to how it is done for the other functions, the documentation
>> only covers the function operating on struct net_device and not struct
>> netdev_queue.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> include/linux/netdevice.h | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 8ed4ae9..ac36629 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -2101,6 +2101,16 @@ static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
>> #endif
>> }
>>
>> +/**
>> + * netdev_sent_queue - report the number of bytes queued to hardware
>> + * @dev: network device
>> + * @bytes: number of bytes queued to the hardware device queue
>> + *
>> + * Report the number of bytes queued for sending/completion to the network
>> + * device hardware queue. @bytes should specify the number of bytes which
>> + * will be sent over the physical medium (without prepended/appended
>> + * control blocks, FCS...)
>
> There is no such requirement.
>
> @bytes should be a good approximation, and should match
> netdev_completed_queue() @bytes
>
>
> If you think of TSO, we know that skb->len does not exactly matches
> number of bytes on physical medium ( check qdisc_pkt_len_init() to see
> how Qdisc layer tries to get better estimation )
Thanks Eric, do you also want me to update the comment above
netdev_completed_queue() or are you happy with it?
--
Florian
^ permalink raw reply
* Re: [Xen-devel] [PATCH] net/core: Order-3 frag allocator causes SWIOTLB bouncing under Xen
From: Konrad Rzeszutek Wilk @ 2013-09-06 13:27 UTC (permalink / raw)
To: Ian Campbell
Cc: Eric Dumazet, Eliezer Tamir, Neil Horman, netdev, linux-kernel,
xen-devel, Eric Dumazet, Li Zefan, david.vrabel, Zoltan Kiss,
malcolm.crossley, David S. Miller
In-Reply-To: <1378366746.6935.35.camel@dagon.hellion.org.uk>
On Thu, Sep 05, 2013 at 08:39:06AM +0100, Ian Campbell wrote:
> On Wed, 2013-09-04 at 17:11 -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Sep 04, 2013 at 02:00:40PM -0700, Eric Dumazet wrote:
>
> > > Maybe you could add proper infrastructure to deal with Xen limitations.
> >
> > I think Ian posted at some point an sysctl patch for that (more for
> > debugging that anything else). And it kind
> > of stalled: http://lists.xen.org/archives/html/xen-devel/2012-10/msg01832.html
>
> I think I though you were looking into it from the swiotlb angle?
Yes. I didn't find anything immediately obvious - but I can still
reproduce with an skge DMA issues when booting baremetal with 'swiotlb=force'.
But only under 32-bit. I think there is some physical address
truncation with the new compound size skb's. Obviously needs
further investigation.
>
> In any case I don't have time to look into this further.
>
> > Is that what you mean by proper infrastructure ?
> >
> > Oh wait, did you mean via dev and not the whole system wide sysctl?
>
> The system wide sysctl was not acceptable AFAIR, which seems reasonable.
<nods>
>
> Per-dev is hard because it affects the native drivers for each physical
> NIC when running under Xen, not the Xen PV NIC which we fixed by
> splitting compound frags into separate slots on the PV ring.
Right. Thank you for pointing that obvious issue.
>
> Ian.
>
^ permalink raw reply
* Re: TSQ accounting skb->truesize degrades throughput for large packets
From: Wei Liu @ 2013-09-06 13:12 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wei Liu, Jonathan Davies, Ian Campbell, netdev, xen-devel
In-Reply-To: <1378472268.31445.15.camel@edumazet-glaptop>
On Fri, Sep 06, 2013 at 05:57:48AM -0700, Eric Dumazet wrote:
> On Fri, 2013-09-06 at 11:16 +0100, Wei Liu wrote:
> > Hi Eric
> >
> > I have some questions regarding TSQ and I hope you can shed some light
> > on this.
> >
> > Our observation is that with the default TSQ limit (128K), throughput
> > for Xen network driver for large packets degrades. That's because we now
> > only have 1 packet in queue.
> >
> > I double-checked that skb->len is indeed <64K. Then I discovered that
> > TSQ actually accounts for skb->truesize and the packets generated had
> > skb->truesize > 64K which effectively prevented us from putting 2
> > packets in queue.
> >
> > There seems to be no way to limit skb->truesize inside driver -- the skb
> > is already constructed when it comes to xen-netfront.
> >
>
> What is the skb->truesize value then ? It must be huge, and its clearly
> a problem, because the tcp _receiver_ will also grow its window slower,
> if packet is looped back.
>
It's ~66KB.
> > My questions are:
> > 1) I see the comment in tcp_output.c saying: "TSQ : sk_wmem_alloc
> > accounts skb truesize, including skb overhead. But thats OK", I
> > don't quite understand why it is OK.
> > 2) presumably other drivers will suffer from this as well, is it
> > possible to account for skb->len instead of skb->truesize?
>
> Well, I have no problem to get line rate on 20Gb with a single flow, so
> other drivers have no problem.
>
OK, good to know this.
> > 3) if accounting skb->truesize is on purpose, does that mean we only
> > need to tune that value instead of trying to fix our driver (if
> > there is a way to)?
>
> The check in TCP allows for two packets at least, unless a single skb
> truesize is 128K ?
>
>
> if (atomic_read(&sk->sk_wmem_alloc) >= sysctl_tcp_limit_output_bytes) {
> set_bit(TSQ_THROTTLED, &tp->tsq_flags);
> break;
> }
>
> So if a skb->truesize is 100K, this condition allows two packets, before
> throttling the third packet.
>
OK. I need to check why we're getting only 1 then.
Thanks for your reply.
Wei.
> Its actually hard to account for skb->len, because sk_wmem_alloc
> accounts for skb->truesize : I do not want to add another
> sk->sk_wbytes_alloc new atomic field.
>
>
^ permalink raw reply
* Re: [PATCH net] bnx2x: Restore a call to config_init
From: Eric Dumazet @ 2013-09-06 13:08 UTC (permalink / raw)
To: eilong; +Cc: David Miller, netdev, Dave Jones
In-Reply-To: <1378461302.15758.2.camel@lb-tlvb-eilong.il.broadcom.com>
On Fri, 2013-09-06 at 12:55 +0300, Eilon Greenstein wrote:
> Commit c0a77ec74f295013d7ba3204dd3ed25fccf83cb4 'bnx2x: Add missing braces in
> bnx2x:bnx2x_link_initialize' identified indentation problem, but resolved it
> by adding braces instead of fixing the indentation. The braces now prevents a
> config_init call in some cases, though it should be called regardless of that
> condition. This patch removes the braces and fix the confusing indentation
> that caused this mess.
>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> CC: Dave Jones <davej@redhat.com>
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
> index 6645684..d60a2ea 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c
> @@ -6501,13 +6501,10 @@ static int bnx2x_link_initialize(struct link_params *params,
> struct bnx2x_phy *phy = ¶ms->phy[INT_PHY];
> if (vars->line_speed == SPEED_AUTO_NEG &&
> (CHIP_IS_E1x(bp) ||
> - CHIP_IS_E2(bp))) {
> + CHIP_IS_E2(bp)))
> bnx2x_set_parallel_detection(phy, params);
> - if (params->phy[INT_PHY].config_init)
> - params->phy[INT_PHY].config_init(phy,
> - params,
> - vars);
> - }
> + if (params->phy[INT_PHY].config_init)
> + params->phy[INT_PHY].config_init(phy, params, vars);
> }
>
> /* Init external phy*/
Thanks
I wish people, including Dave Jones, clearly states that a patch was not
actually tested on real hardware.
Tested-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: GSO/GRO and UDP performance
From: Eric Dumazet @ 2013-09-06 13:07 UTC (permalink / raw)
To: James Yonan; +Cc: netdev
In-Reply-To: <52299EDD.1030208@openvpn.net>
On Fri, 2013-09-06 at 03:22 -0600, James Yonan wrote:
> So I think that playing well with GSO/GRO is essential to get speedup in
> UDP apps because of this 43x multiplier.
>
Thats not true. GRO cannot aggregate more than 16+1 packets.
I think we cannot aggregate UDP packets, because UDP lacks sequence
numbers, so reorders would be a problem.
You really need something that is not UDP generic.
^ permalink raw reply
* Re: [PATCH net-next] net: add documentation for BQL helpers
From: Eric Dumazet @ 2013-09-06 13:03 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, davem
In-Reply-To: <1378465119-1894-1-git-send-email-f.fainelli@gmail.com>
On Fri, 2013-09-06 at 11:58 +0100, Florian Fainelli wrote:
> Provide a kernel-doc comment documentation for the BQL helpers:
> - netdev_sent_queue
> - netdev_completed_queue
> - netdev_reset_queue
>
> Similarly to how it is done for the other functions, the documentation
> only covers the function operating on struct net_device and not struct
> netdev_queue.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> include/linux/netdevice.h | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8ed4ae9..ac36629 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2101,6 +2101,16 @@ static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
> #endif
> }
>
> +/**
> + * netdev_sent_queue - report the number of bytes queued to hardware
> + * @dev: network device
> + * @bytes: number of bytes queued to the hardware device queue
> + *
> + * Report the number of bytes queued for sending/completion to the network
> + * device hardware queue. @bytes should specify the number of bytes which
> + * will be sent over the physical medium (without prepended/appended
> + * control blocks, FCS...)
There is no such requirement.
@bytes should be a good approximation, and should match
netdev_completed_queue() @bytes
If you think of TSO, we know that skb->len does not exactly matches
number of bytes on physical medium ( check qdisc_pkt_len_init() to see
how Qdisc layer tries to get better estimation )
> + */
> static inline void netdev_sent_queue(struct net_device *dev, unsigned int bytes)
> {
> netdev_tx_sent_queue(netdev_get_tx_queue(dev, 0), bytes);
> @@ -2130,6 +2140,16 @@ static inline void netdev_tx_completed_queue(struct netdev_queue *dev_queue,
> #endif
> }
>
> +/**
> + * netdev_completed_queue - report bytes and packets completed by device
> + * @dev: network device
> + * @pkts: actual number of packets sent over the medium
> + * @bytes: actual number of bytes sent over the medium
> + *
> + * Report the number of bytes and packets transmitted by the network device
> + * hardware queue over the physical medium (without prepended/appended
> + * control blocks, FCS...)
> + */
^ permalink raw reply
* Re: TSQ accounting skb->truesize degrades throughput for large packets
From: Eric Dumazet @ 2013-09-06 12:57 UTC (permalink / raw)
To: Wei Liu; +Cc: Jonathan Davies, Ian Campbell, netdev, xen-devel
In-Reply-To: <20130906101635.GI14104@zion.uk.xensource.com>
On Fri, 2013-09-06 at 11:16 +0100, Wei Liu wrote:
> Hi Eric
>
> I have some questions regarding TSQ and I hope you can shed some light
> on this.
>
> Our observation is that with the default TSQ limit (128K), throughput
> for Xen network driver for large packets degrades. That's because we now
> only have 1 packet in queue.
>
> I double-checked that skb->len is indeed <64K. Then I discovered that
> TSQ actually accounts for skb->truesize and the packets generated had
> skb->truesize > 64K which effectively prevented us from putting 2
> packets in queue.
>
> There seems to be no way to limit skb->truesize inside driver -- the skb
> is already constructed when it comes to xen-netfront.
>
What is the skb->truesize value then ? It must be huge, and its clearly
a problem, because the tcp _receiver_ will also grow its window slower,
if packet is looped back.
> My questions are:
> 1) I see the comment in tcp_output.c saying: "TSQ : sk_wmem_alloc
> accounts skb truesize, including skb overhead. But thats OK", I
> don't quite understand why it is OK.
> 2) presumably other drivers will suffer from this as well, is it
> possible to account for skb->len instead of skb->truesize?
Well, I have no problem to get line rate on 20Gb with a single flow, so
other drivers have no problem.
> 3) if accounting skb->truesize is on purpose, does that mean we only
> need to tune that value instead of trying to fix our driver (if
> there is a way to)?
The check in TCP allows for two packets at least, unless a single skb
truesize is 128K ?
if (atomic_read(&sk->sk_wmem_alloc) >= sysctl_tcp_limit_output_bytes) {
set_bit(TSQ_THROTTLED, &tp->tsq_flags);
break;
}
So if a skb->truesize is 100K, this condition allows two packets, before
throttling the third packet.
Its actually hard to account for skb->len, because sk_wmem_alloc
accounts for skb->truesize : I do not want to add another
sk->sk_wbytes_alloc new atomic field.
^ permalink raw reply
* Re: [PATCH net-next v4 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Nikolay Aleksandrov @ 2013-09-06 11:59 UTC (permalink / raw)
To: Ding Tianhong
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Veaceslav Falico,
Netdev
In-Reply-To: <52298407.9040103@huawei.com>
On 09/06/2013 09:28 AM, Ding Tianhong wrote:
> The commit 278b20837511776dc9d5f6ee1c7fabd5479838bb
> (bonding: initial RCU conversion) has convert the roundrobin, active-backup,
> broadcast and xor xmit path to rcu protection, the performance will be better
> for these mode, so this time, convert xmit path for 3ad mode.
>
> Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
> Cc: Veaceslav Falico <vfalico@redhat.com>
> ---
> drivers/net/bonding/bond_3ad.c | 32 ++++++++++++++------------------
> drivers/net/bonding/bonding.h | 33 ++++++++++++++++++++++++++++++++-
> 2 files changed, 46 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 0d8f427..13f1deb 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -143,7 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
> */
> static inline struct port *__get_first_port(struct bonding *bond)
> {
> - struct slave *first_slave = bond_first_slave(bond);
> + struct slave *first_slave = bond_first_slave_rcu(bond);
>
> return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
> }
> @@ -163,7 +163,7 @@ static inline struct port *__get_next_port(struct port *port)
> // If there's no bond for this port, or this is the last slave
> if (bond == NULL)
> return NULL;
> - slave_next = bond_next_slave(bond, slave);
> + slave_next = bond_next_slave_rcu(bond, slave);
> if (!slave_next || bond_is_first_slave(bond, slave_next))
> return NULL;
>
> @@ -2417,16 +2417,14 @@ int bond_3ad_get_active_agg_info(struct bonding *bond, struct ad_info *ad_info)
>
> int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
> {
> - struct slave *slave, *start_at;
> struct bonding *bond = netdev_priv(dev);
> + struct slave *slave;
> int slave_agg_no;
> int slaves_in_agg;
> int agg_id;
> - int i;
> struct ad_info ad_info;
> int res = 1;
>
> - read_lock(&bond->lock);
> if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
> pr_debug("%s: Error: __bond_3ad_get_active_agg_info failed\n",
> dev->name);
> @@ -2444,13 +2442,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>
> slave_agg_no = bond->xmit_hash_policy(skb, slaves_in_agg);
>
> - bond_for_each_slave(bond, slave) {
> + bond_for_each_slave_rcu(bond, slave) {
> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>
> if (agg && (agg->aggregator_identifier == agg_id)) {
> - slave_agg_no--;
> - if (slave_agg_no < 0)
> - break;
> + if (--slave_agg_no < 0) {
> + if (SLAVE_IS_OK(slave)) {
> + res = bond_dev_queue_xmit(bond,
> + skb, slave->dev);
> + goto out;
> + }
> + }
> }
> }
>
> @@ -2460,23 +2462,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
> goto out;
> }
>
> - start_at = slave;
> -
> - bond_for_each_slave_from(bond, slave, i, start_at) {
> - int slave_agg_id = 0;
> + bond_for_each_slave_rcu(bond, slave) {
> struct aggregator *agg = SLAVE_AD_INFO(slave).port.aggregator;
>
> - if (agg)
> - slave_agg_id = agg->aggregator_identifier;
> -
> - if (SLAVE_IS_OK(slave) && agg && (slave_agg_id == agg_id)) {
> + if (SLAVE_IS_OK(slave) && agg &&
> + agg->aggregator_identifier == agg_id) {
> res = bond_dev_queue_xmit(bond, skb, slave->dev);
> break;
> }
> }
>
> out:
> - read_unlock(&bond->lock);
> if (res) {
> /* no suitable interface, frame not sent */
> kfree_skb(skb);
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index f7ab161..419161d 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -74,13 +74,35 @@
> /* slave list primitives */
> #define bond_to_slave(ptr) list_entry(ptr, struct slave, list)
>
> +/* slave list primitives, Caller must hold rcu_read_lock */
> +#define bond_to_slave_rcu(ptr) list_entry_rcu(ptr, struct slave, list)
> +
> +/* bond_is_empty return NULL if slave list is empty*/
> +#define bond_is_empty(bond) \
> + (list_empty(&(bond)->slave_list))
> +
> +/* bond_is_empty_rcu return NULL if slave list is empty*/
> +#define bond_is_empty_rcu(bond) \
> + (!list_first_or_null_rcu(&(bond)->slave_list, struct slave, list))
> +
> /* IMPORTANT: bond_first/last_slave can return NULL in case of an empty list */
> #define bond_first_slave(bond) \
> list_first_entry_or_null(&(bond)->slave_list, struct slave, list)
> #define bond_last_slave(bond) \
> - (list_empty(&(bond)->slave_list) ? NULL : \
> + (bond_is_empty(bond) ? NULL : \
> bond_to_slave((bond)->slave_list.prev))
>
> +/**
> + * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of an empty list
> + * Caller must hold rcu_read_lock
> + */
> +#define bond_first_slave_rcu(bond) \
> + (bond_is_empty_rcu(bond) ? NULL : \
> + bond_to_slave_rcu((bond)->slave_list.next))
> +#define bond_last_slave_rcu(bond) \
> + (bond_is_empty_rcu(bond) ? NULL : \
> + bond_to_slave_rcu((bond)->slave_list.prev))
> +
This still has the bug that you and Veaceslav were discussing earlier.
To be honest, I'm getting tired of these fast re-posts without any actual
changes, it really is really starting to get on my nerves. Please before posting
the next version take some time (more time) re-think it, go over it more times,
don't go with the first thing that comes to mind without thinking it through well.
Now to be specific here for the Nth time:
You _can't_ do this sequence:
if (list_first_or_null_rcu()) -> rcu_dereference(first/last element) because
between the check which actually dereferences it and the second dereference the
first/last element might be long gone. You should use the result of
list_first_or_null_rcu.
Cheers,
Nik
> #define bond_is_first_slave(bond, pos) ((pos)->list.prev == &(bond)->slave_list)
> #define bond_is_last_slave(bond, pos) ((pos)->list.next == &(bond)->slave_list)
>
> @@ -93,6 +115,15 @@
> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
> bond_to_slave((pos)->list.prev))
>
> +/* Since bond_first/last_slave_rcu can return NULL, these can return NULL too */
> +#define bond_next_slave_rcu(bond, pos) \
> + (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \
> + bond_to_slave_rcu((pos)->list.next))
> +
P.S. Either I'm getting paranoid or I think there's the same bug here, namely:
say list.next != slave_list, but at the time of dereferencing list.next
bond_to_slave_rcu() can get slave_list if it was changed.
> +#define bond_prev_slave_rcu(bond, pos) \
> + (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
> + bond_to_slave_rcu((pos)->list.prev))
> +
> /**
> * bond_for_each_slave_from - iterate the slaves list from a starting point
> * @bond: the bond holding this list.
>
^ permalink raw reply
* Re: [PATCH net-next v4 5/6] bonding: restructure and add rcu for bond_for_each_slave_next()
From: Nikolay Aleksandrov @ 2013-09-06 11:59 UTC (permalink / raw)
To: Ding Tianhong
Cc: Jay Vosburgh, Andy Gospodarek, David S. Miller, Veaceslav Falico,
Netdev
In-Reply-To: <5229841E.4020008@huawei.com>
On 09/06/2013 09:28 AM, Ding Tianhong wrote:
> Restructure the bond_for_each_slave(), remove the checking for bond->slave_cnt,
> make the new loop be more simple and racy.
>
> Add bond_for_each_slave_next_rcu() for next patch use.
>
> Suggested-by: Nikolay Aleksandrov <nikolay@redhat.com>
> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> Cc: Nikolay Aleksandrov <nikolay@redhat.com>
> Cc: Veaceslav Falico <vfalico@redhat.com>
> ---
> drivers/net/bonding/bond_alb.c | 3 +--
> drivers/net/bonding/bond_main.c | 6 ++----
> drivers/net/bonding/bonding.h | 21 +++++++++++++++++----
> 3 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 91f179d..c75d383 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -383,7 +383,6 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
> {
> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> struct slave *rx_slave, *slave, *start_at;
> - int i = 0;
>
> if (bond_info->next_rx_slave)
> start_at = bond_info->next_rx_slave;
> @@ -392,7 +391,7 @@ static struct slave *rlb_next_rx_slave(struct bonding *bond)
>
> rx_slave = NULL;
>
> - bond_for_each_slave_from(bond, slave, i, start_at) {
> + bond_for_each_slave_from(bond, slave, start_at) {
> if (SLAVE_IS_OK(slave)) {
> if (!rx_slave) {
> rx_slave = slave;
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 39e5b1c..4190389 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -782,7 +782,6 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
> struct slave *new_active, *old_active;
> struct slave *bestslave = NULL;
> int mintime = bond->params.updelay;
> - int i;
>
> new_active = bond->curr_active_slave;
>
> @@ -801,7 +800,7 @@ static struct slave *bond_find_best_slave(struct bonding *bond)
> /* remember where to stop iterating over the slaves */
> old_active = new_active;
>
> - bond_for_each_slave_from(bond, new_active, i, old_active) {
> + bond_for_each_slave_from(bond, new_active, old_active) {
> if (new_active->link == BOND_LINK_UP) {
> return new_active;
> } else if (new_active->link == BOND_LINK_BACK &&
> @@ -2756,7 +2755,6 @@ do_failover:
> static void bond_ab_arp_probe(struct bonding *bond)
> {
> struct slave *slave, *next_slave;
> - int i;
>
> read_lock(&bond->curr_slave_lock);
>
> @@ -2788,7 +2786,7 @@ static void bond_ab_arp_probe(struct bonding *bond)
>
> /* search for next candidate */
> next_slave = bond_next_slave(bond, bond->current_arp_slave);
> - bond_for_each_slave_from(bond, slave, i, next_slave) {
> + bond_for_each_slave_from(bond, slave, next_slave) {
> if (IS_UP(slave->dev)) {
> slave->link = BOND_LINK_BACK;
> bond_set_slave_active_flags(slave);
> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> index 419161d..d80e146 100644
> --- a/drivers/net/bonding/bonding.h
> +++ b/drivers/net/bonding/bonding.h
> @@ -124,18 +124,31 @@
> (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
> bond_to_slave_rcu((pos)->list.prev))
>
> +/* Check whether the slave is the only one in bond */
> +
> /**
> * bond_for_each_slave_from - iterate the slaves list from a starting point
> * @bond: the bond holding this list.
> * @pos: current slave.
> - * @cnt: counter for max number of moves
> * @start: starting point.
> *
> * Caller must hold bond->lock
> */
> -#define bond_for_each_slave_from(bond, pos, cnt, start) \
> - for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \
> - cnt++, pos = bond_next_slave(bond, pos))
> +#define bond_for_each_slave_from(bond, pos, start) \
> + for (pos = start; pos; (pos = bond_next_slave(bond, pos)) != start ? \
> + (pos) : (pos = NULL))
> +
> +/**
> + * bond_for_each_slave_from_rcu - iterate the slaves list from a starting point
> + * @bond: the bond holding this list.
> + * @pos: current slave.
> + * @start: starting point.
> + *
> + * Caller must hold rcu_read_lock
> + */
> +#define bond_for_each_slave_from_rcu(bond, pos, start) \
> + for (pos = start; pos; (pos = bond_next_slave_rcu(bond, pos)) != start ? \
> + (pos) : (pos = NULL))
>
The same bug here as before... What happens if start disappears ? - Infinite
loop, pos will never be equal to start again.
> /**
> * bond_for_each_slave - iterate over all slaves
>
^ permalink raw reply
* [PATCH] mlx5: remove unused MLX5_DEBUG param in Kconfig
From: Michael Opdenacker @ 2013-09-06 11:49 UTC (permalink / raw)
To: eli-VPRAkNaXOzVWk0Htik3J/w
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Michael Opdenacker
This patch proposes to remove the MLX5_DEBUG kernel configuration
parameter defined in drivers/net/ethernet/mellanox/mlx5/core/Kconfig,
but used nowhere in the makefiles and source code.
This could also be fixed by using this parameter,
but this may be a leftover from driver development...
Signed-off-by: Michael Opdenacker <michael.opdenacker-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
drivers/net/ethernet/mellanox/mlx5/core/Kconfig | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index 2196282..157fe8d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -6,13 +6,3 @@ config MLX5_CORE
tristate
depends on PCI && X86
default n
-
-config MLX5_DEBUG
- bool "Verbose debugging output" if (MLX5_CORE && EXPERT)
- depends on MLX5_CORE
- default y
- ---help---
- This option causes debugging code to be compiled into the
- mlx5_core driver. The output can be turned on via the
- debug_mask module parameter (which can also be set after
- the driver is loaded through sysfs).
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH net-next] net: add documentation for BQL helpers
From: Florian Fainelli @ 2013-09-06 10:58 UTC (permalink / raw)
To: netdev; +Cc: eric.dumazet, davem, Florian Fainelli
Provide a kernel-doc comment documentation for the BQL helpers:
- netdev_sent_queue
- netdev_completed_queue
- netdev_reset_queue
Similarly to how it is done for the other functions, the documentation
only covers the function operating on struct net_device and not struct
netdev_queue.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
include/linux/netdevice.h | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8ed4ae9..ac36629 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2101,6 +2101,16 @@ static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
#endif
}
+/**
+ * netdev_sent_queue - report the number of bytes queued to hardware
+ * @dev: network device
+ * @bytes: number of bytes queued to the hardware device queue
+ *
+ * Report the number of bytes queued for sending/completion to the network
+ * device hardware queue. @bytes should specify the number of bytes which
+ * will be sent over the physical medium (without prepended/appended
+ * control blocks, FCS...)
+ */
static inline void netdev_sent_queue(struct net_device *dev, unsigned int bytes)
{
netdev_tx_sent_queue(netdev_get_tx_queue(dev, 0), bytes);
@@ -2130,6 +2140,16 @@ static inline void netdev_tx_completed_queue(struct netdev_queue *dev_queue,
#endif
}
+/**
+ * netdev_completed_queue - report bytes and packets completed by device
+ * @dev: network device
+ * @pkts: actual number of packets sent over the medium
+ * @bytes: actual number of bytes sent over the medium
+ *
+ * Report the number of bytes and packets transmitted by the network device
+ * hardware queue over the physical medium (without prepended/appended
+ * control blocks, FCS...)
+ */
static inline void netdev_completed_queue(struct net_device *dev,
unsigned int pkts, unsigned int bytes)
{
@@ -2144,6 +2164,13 @@ static inline void netdev_tx_reset_queue(struct netdev_queue *q)
#endif
}
+/**
+ * netdev_reset_queue - reset the packets and bytes count of a network device
+ * @dev_queue: network device
+ *
+ * Reset the bytes and packet count of a network device and clear the
+ * software flow control OFF bit for this network device
+ */
static inline void netdev_reset_queue(struct net_device *dev_queue)
{
netdev_tx_reset_queue(netdev_get_tx_queue(dev_queue, 0));
--
1.8.1.2
^ permalink raw reply related
* RE: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.
From: David Laight @ 2013-09-06 10:18 UTC (permalink / raw)
To: Jesse Gross, David Miller
Cc: netdev, dev, Andy Zhou, Fengguang Wu, Geert Uytterhoeven
In-Reply-To: <1378402887-17331-1-git-send-email-jesse@nicira.com>
> -} __aligned(__alignof__(long));
> +} __aligned(8); /* 8 byte alignment ensures this can be accessed as a long */
Don't you really want __aligned(MAX(__alignof__(__be64), sizeof (long))) ?
David
^ 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