* Re: [PATCH] net: ethernet: freescale: fec_main.c: Cleaning up missing null-terminate in conjunction with strncpy
From: David Miller @ 2014-09-15 18:17 UTC (permalink / raw)
To: rickard_strandqvist
Cc: grant.likely, robh+dt, B38611, rmk+kernel, fabio.estevam,
frank.li, u.kleine-koenig, netdev, linux-kernel, devicetree
In-Reply-To: <1410715962-15986-1-git-send-email-rickard_strandqvist@spectrumdigital.se>
From: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Date: Sun, 14 Sep 2014 19:32:42 +0200
> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
Applied.
^ permalink raw reply
* [Patch net-next 4/4] net: fec: Workaround for imx6sx enet tx hang when enable three queues
From: Frank.Li @ 2014-09-15 17:12 UTC (permalink / raw)
To: b38611, davem, netdev, lznuaa
Cc: shawn.guo, linux-arm-kernel, Fugang Duan, Frank Li
In-Reply-To: <1410801177-15872-1-git-send-email-Frank.Li@freescale.com>
From: Fugang Duan <B38611@freescale.com>
When enable three queues on imx6sx enet, and then do tx performance
test with iperf tool, after some time running, tx hang.
Found that:
If uDMA is running, software set TDAR may cause tx hang.
If uDMA is in idle, software set TDAR don't cause tx hang.
There is a TDAR race condition for mutliQ when the software sets TDAR
and the UDMA clears TDAR simultaneously or in a small window (2-4 cycles).
This will cause the udma_tx and udma_tx_arbiter state machines to hang.
The issue exist at i.MX6SX enet IP.
So, the Workaround is checking TDAR status four time, if TDAR cleared by
hardware and then write TDAR, otherwise don't set TDAR.
The patch is only one Workaround for the issue TKT210582.
Signed-off-by: Fugang Duan <B38611@freescale.com>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
drivers/net/ethernet/freescale/fec_main.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 524ddbe..452e576 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -111,6 +111,13 @@ static void fec_enet_itr_coal_init(struct net_device *ndev);
* independent rings
*/
#define FEC_QUIRK_HAS_AVB (1 << 8)
+/*
+ * There is a TDAR race condition for mutliQ when the software sets TDAR
+ * and the UDMA clears TDAR simultaneously or in a small window (2-4 cycles).
+ * This will cause the udma_tx and udma_tx_arbiter state machines to hang.
+ * The issue exist at i.MX6SX enet IP.
+ */
+#define FEC_QUIRK_TKT210582 (1 << 9)
static struct platform_device_id fec_devtype[] = {
{
@@ -139,7 +146,7 @@ static struct platform_device_id fec_devtype[] = {
.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
FEC_QUIRK_HAS_VLAN | FEC_QUIRK_ERR006358 |
- FEC_QUIRK_HAS_AVB,
+ FEC_QUIRK_HAS_AVB | FEC_QUIRK_TKT210582,
}, {
/* sentinel */
}
@@ -709,6 +716,8 @@ static int fec_enet_txq_submit_tso(struct fec_enet_priv_tx_q *txq,
struct tso_t tso;
unsigned int index = 0;
int ret;
+ const struct platform_device_id *id_entry =
+ platform_get_device_id(fep->pdev);
if (tso_count_descs(skb) >= fec_enet_get_free_txdesc_num(fep, txq)) {
dev_kfree_skb_any(skb);
@@ -770,7 +779,12 @@ static int fec_enet_txq_submit_tso(struct fec_enet_priv_tx_q *txq,
txq->cur_tx = bdp;
/* Trigger transmission start */
- writel(0, fep->hwp + FEC_X_DES_ACTIVE(queue));
+ if (!(id_entry->driver_data & FEC_QUIRK_TKT210582) ||
+ !readl(fep->hwp + FEC_X_DES_ACTIVE(queue)) ||
+ !readl(fep->hwp + FEC_X_DES_ACTIVE(queue)) ||
+ !readl(fep->hwp + FEC_X_DES_ACTIVE(queue)) ||
+ !readl(fep->hwp + FEC_X_DES_ACTIVE(queue)))
+ writel(0, fep->hwp + FEC_X_DES_ACTIVE(queue));
return 0;
--
1.9.1
^ permalink raw reply related
* [Patch net-next 2/4] net: fec: add interrupt coalescence feature support
From: Frank.Li @ 2014-09-15 17:12 UTC (permalink / raw)
To: b38611, davem, netdev, lznuaa
Cc: shawn.guo, linux-arm-kernel, Fugang Duan, Frank Li
In-Reply-To: <1410801177-15872-1-git-send-email-Frank.Li@freescale.com>
From: Fugang Duan <B38611@freescale.com>
i.MX6 SX support interrupt coalescence feature
By default, init the interrupt coalescing frame count threshold and
timer threshold.
Supply the ethtool interfaces as below for user tuning to improve
enet performance:
rx_max_coalesced_frames
rx_coalesce_usecs
tx_max_coalesced_frames
tx_coalesce_usecs
Signed-off-by: Fugang Duan <B38611@freescale.com>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
drivers/net/ethernet/freescale/fec.h | 14 ++++
drivers/net/ethernet/freescale/fec_main.c | 111 ++++++++++++++++++++++++++++++
2 files changed, 125 insertions(+)
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index b7c7722..f13e319 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -335,6 +335,14 @@ struct bufdesc_ex {
#define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII | FEC_ENET_TS_TIMER)
#define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
+/* ENET interrupt coalescing macro define */
+#define FEC_ITR_CLK_SEL (0x1 << 30)
+#define FEC_ITR_EN (0x1 << 31)
+#define FEC_ITR_ICFT(X) ((X & 0xFF) << 20)
+#define FEC_ITR_ICTT(X) ((X) & 0xFFFF)
+#define FEC_ITR_ICFT_DEFAULT 200 /* Set 200 frame count threshold */
+#define FEC_ITR_ICTT_DEFAULT 1000 /* Set 1000us timer threshold */
+
#define FEC_VLAN_TAG_LEN 0x04
#define FEC_ETHTYPE_LEN 0x02
@@ -446,6 +454,12 @@ struct fec_enet_private {
unsigned int tx_align;
unsigned int rx_align;
+
+ /* hw interrupt coalesce */
+ uint rx_pkts_itr;
+ uint rx_time_itr;
+ uint tx_pkts_itr;
+ uint tx_time_itr;
};
void fec_ptp_init(struct platform_device *pdev);
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index f958e91..524ddbe 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -63,6 +63,7 @@
#include "fec.h"
static void set_multicast_list(struct net_device *ndev);
+static void fec_enet_itr_coal_init(struct net_device *ndev);
#define DRIVER_NAME "fec"
@@ -1095,6 +1096,10 @@ fec_restart(struct net_device *ndev)
/* Enable interrupts we wish to service */
writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
+
+ /* Init the interrupt coalescing */
+ fec_enet_itr_coal_init(ndev);
+
}
static void
@@ -2234,12 +2239,118 @@ static int fec_enet_nway_reset(struct net_device *dev)
return genphy_restart_aneg(phydev);
}
+/*
+ * ITR clock source is enet system clock (clk_ahb).
+ * TCTT unit is cycle_ns * 64 cycle
+ * So, the ICTT value = X us / (cycle_ns * 64)
+ */
+static int fec_enet_us_to_itr_clock(struct net_device *ndev, int us)
+{
+ struct fec_enet_private *fep = netdev_priv(ndev);
+
+ return us * (clk_get_rate(fep->clk_ahb) / 64000) / 1000;
+}
+
+/* Set threshold for interrupt coalescing */
+static void fec_enet_itr_coal_set(struct net_device *ndev)
+{
+ struct fec_enet_private *fep = netdev_priv(ndev);
+ const struct platform_device_id *id_entry =
+ platform_get_device_id(fep->pdev);
+ int rx_itr, tx_itr;
+
+ if (!(id_entry->driver_data & FEC_QUIRK_HAS_AVB))
+ return;
+
+ /* Must be greater than zero to avoid unpredictable behavior */
+ if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
+ !fep->tx_time_itr || !fep->tx_pkts_itr)
+ return;
+ /*
+ * Select enet system clock as Interrupt Coalescing
+ * timer Clock Source
+ */
+ rx_itr = FEC_ITR_CLK_SEL;
+ tx_itr = FEC_ITR_CLK_SEL;
+
+ /* set ICFT and ICTT */
+ rx_itr |= FEC_ITR_ICFT(fep->rx_pkts_itr);
+ rx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev, fep->rx_time_itr));
+ tx_itr |= FEC_ITR_ICFT(fep->tx_pkts_itr);
+ tx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev, fep->tx_time_itr));
+
+ rx_itr |= FEC_ITR_EN;
+ tx_itr |= FEC_ITR_EN;
+
+ writel(tx_itr, fep->hwp + FEC_TXIC0);
+ writel(rx_itr, fep->hwp + FEC_RXIC0);
+ writel(tx_itr, fep->hwp + FEC_TXIC1);
+ writel(rx_itr, fep->hwp + FEC_RXIC1);
+ writel(tx_itr, fep->hwp + FEC_TXIC2);
+ writel(rx_itr, fep->hwp + FEC_RXIC2);
+}
+
+static int fec_enet_get_coalesce(struct net_device *ndev,
+ struct ethtool_coalesce *ec)
+{
+ struct fec_enet_private *fep = netdev_priv(ndev);
+ const struct platform_device_id *id_entry =
+ platform_get_device_id(fep->pdev);
+
+ if (!(id_entry->driver_data & FEC_QUIRK_HAS_AVB))
+ return -EOPNOTSUPP;
+
+ ec->rx_coalesce_usecs = fep->rx_time_itr;
+ ec->rx_max_coalesced_frames = fep->rx_pkts_itr;
+
+ ec->tx_coalesce_usecs = fep->tx_time_itr;
+ ec->tx_max_coalesced_frames = fep->tx_pkts_itr;
+
+ return 0;
+}
+
+static int fec_enet_set_coalesce(struct net_device *ndev,
+ struct ethtool_coalesce *ec)
+{
+ struct fec_enet_private *fep = netdev_priv(ndev);
+ const struct platform_device_id *id_entry =
+ platform_get_device_id(fep->pdev);
+
+ if (!(id_entry->driver_data & FEC_QUIRK_HAS_AVB))
+ return -EOPNOTSUPP;
+
+ fep->rx_time_itr = ec->rx_coalesce_usecs;
+ fep->rx_pkts_itr = ec->rx_max_coalesced_frames;
+
+ fep->tx_time_itr = ec->tx_coalesce_usecs;
+ fep->tx_pkts_itr = ec->tx_max_coalesced_frames;
+
+ fec_enet_itr_coal_set(ndev);
+
+ return 0;
+}
+
+static void fec_enet_itr_coal_init(struct net_device *ndev)
+{
+ struct ethtool_coalesce ec;
+
+ ec.rx_coalesce_usecs = FEC_ITR_ICTT_DEFAULT;
+ ec.rx_max_coalesced_frames = FEC_ITR_ICFT_DEFAULT;
+
+ ec.tx_coalesce_usecs = FEC_ITR_ICTT_DEFAULT;
+ ec.tx_max_coalesced_frames = FEC_ITR_ICFT_DEFAULT;
+
+ fec_enet_set_coalesce(ndev, &ec);
+}
+
static const struct ethtool_ops fec_enet_ethtool_ops = {
.get_settings = fec_enet_get_settings,
.set_settings = fec_enet_set_settings,
.get_drvinfo = fec_enet_get_drvinfo,
.nway_reset = fec_enet_nway_reset,
.get_link = ethtool_op_get_link,
+ .get_coalesce = fec_enet_get_coalesce,
+ .set_coalesce = fec_enet_set_coalesce,
#ifndef CONFIG_M5272
.get_pauseparam = fec_enet_get_pauseparam,
.set_pauseparam = fec_enet_set_pauseparam,
--
1.9.1
^ permalink raw reply related
* [Patch net-next 1/4] net: fec: refine error handle of parser queue number from DT
From: Frank.Li @ 2014-09-15 17:12 UTC (permalink / raw)
To: b38611, davem, netdev, lznuaa; +Cc: shawn.guo, linux-arm-kernel, Frank Li
In-Reply-To: <1410801177-15872-1-git-send-email-Frank.Li@freescale.com>
From: Frank Li <Frank.Li@freescale.com>
check tx and rx queue seperately.
fix typo, "Invalidate" and "fail".
change pr_err to pr_warn.
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
drivers/net/ethernet/freescale/fec_main.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 8f8e55e..f958e91 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2890,22 +2890,22 @@ fec_enet_get_queue_num(struct platform_device *pdev, int *num_tx, int *num_rx)
/* parse the num of tx and rx queues */
err = of_property_read_u32(np, "fsl,num-tx-queues", num_tx);
- err |= of_property_read_u32(np, "fsl,num-rx-queues", num_rx);
- if (err) {
+ if (err)
*num_tx = 1;
+
+ err = of_property_read_u32(np, "fsl,num-rx-queues", num_rx);
+ if (err)
*num_rx = 1;
- return;
- }
if (*num_tx < 1 || *num_tx > FEC_ENET_MAX_TX_QS) {
- dev_err(&pdev->dev, "Invalidate num_tx(=%d), fail back to 1\n",
+ dev_warn(&pdev->dev, "Invalid num_tx(=%d), fall back to 1\n",
*num_tx);
*num_tx = 1;
return;
}
if (*num_rx < 1 || *num_rx > FEC_ENET_MAX_RX_QS) {
- dev_err(&pdev->dev, "Invalidate num_rx(=%d), fail back to 1\n",
+ dev_warn(&pdev->dev, "Invalid num_rx(=%d), fall back to 1\n",
*num_rx);
*num_rx = 1;
return;
@@ -2924,8 +2924,8 @@ fec_probe(struct platform_device *pdev)
const struct of_device_id *of_id;
static int dev_id;
struct device_node *np = pdev->dev.of_node, *phy_node;
- int num_tx_qs = 1;
- int num_rx_qs = 1;
+ int num_tx_qs;
+ int num_rx_qs;
of_id = of_match_device(fec_dt_ids, &pdev->dev);
if (of_id)
--
1.9.1
^ permalink raw reply related
* [Patch net-next 0/4] net: fec: add interrupt coalescence
From: Frank.Li @ 2014-09-15 17:12 UTC (permalink / raw)
To: b38611, davem, netdev, lznuaa; +Cc: shawn.guo, linux-arm-kernel, Frank Li
From: Frank Li <Frank.Li@freescale.com>
improve error handle when parse queue number.
add interrupt coalescence feature.
Frank Li (1):
net: fec: refine error handle of parser queue number from DT
Fugang Duan (3):
net: fec: add interrupt coalescence feature support
net:fec: increase DMA queue number
net: fec: Workaround for imx6sx enet tx hang when enable three queues
drivers/net/ethernet/freescale/fec.h | 16 +++-
drivers/net/ethernet/freescale/fec_main.c | 144 ++++++++++++++++++++++++++++--
2 files changed, 150 insertions(+), 10 deletions(-)
--
1.9.1
^ permalink raw reply
* H
From: Diana @ 2014-09-15 16:59 UTC (permalink / raw)
Please Revert back, your assistance is needed.
---------------------------------------
The Exhibitor at innoTrans, Berlin 2014
Hall : 15.1 / Stand no : 109
http://www.virtualmarket.innotrans.de/?Action=showCompany&id=346242
^ permalink raw reply
* Re: [PATCH v2 net-next 0/7] net: foo-over-udp (fou)
From: Or Gerlitz @ 2014-09-15 18:08 UTC (permalink / raw)
To: Tom Herbert; +Cc: David Miller, Linux Netdev List
In-Reply-To: <1410750483-3236-1-git-send-email-therbert@google.com>
On Mon, Sep 15, 2014 at 6:07 AM, Tom Herbert <therbert@google.com> wrote:
[...]
> * Notes
> - This patch set does not implement GSO for FOU. The UDP encapsulation
> code assumes TEB, so that will need to be reimplemented.
Can you please clarify this point little further? Specifically, today
few NICs are
advertizing NETIF_F_GSO_UDP_TUNNEL when they are practically GSO
capable only w.r.t to VXLAN. What happens when such NIC expose this
cap and a large guest frame goes through GRE over UDP or alike tunneling?
> - I really don't expect/want devices to have special support for any
> of this. Generic checksum offload mechanisms (NETIF_HW_CSUM
> and use of CHECKSUM_COMPLETE) should be sufficient. RSS and flow
> steering is provided by commonly implemented UDP hashing. GRO/GSO
> seem fairly comparable with LRO/TSO already.
Again, today NICs are advertizing checksum offloads capability in
enc_hw_features but aren't capable to compute (say) the TCP checksum
of the inner
packet regardless of which actual tunneling is used (e.g VXLAN vs
GRE), a bit inconsistent?
^ permalink raw reply
* Re: [PATCH net-next v3 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: Mahesh Bandewar @ 2014-09-15 18:05 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David Miller,
netdev, Eric Dumazet, Maciej Zenczykowski
In-Reply-To: <CAF2d9jiEdZvaGnhd6QNgY5yaNdrNu556Ctz-ZUwotpA781GubQ@mail.gmail.com>
Even though the patch is working correctly (me and Nikolay have
resolved the vm vs. non-vm issues), there is a race situation created
since the lock-simplification patches are applied. We would resolve
this before this patch can be applied. Probably the first patch in
this series is stuck unnecessarily, so I will disassociate that patch
and send it separately.
On Mon, Sep 15, 2014 at 11:00 AM, Mahesh Bandewar <maheshb@google.com> wrote:
> Even though the patch is working correctly (me and Nikolay have resolved the
> vm vs. non-vm issues), there is a race situation created since the
> lock-simplification patches are applied. We would resolve this before this
> patch can be applied. Probably the first patch in this series is stuck
> unnecessarily, so I will disassociate that patch and send it separately.
>
> On Thu, Sep 11, 2014 at 3:44 PM, Nikolay Aleksandrov <nikolay@redhat.com>
> wrote:
>>
>> On 09/12/2014 12:08 AM, Mahesh Bandewar wrote:
>> > some how my earlier mail bounced back (formatting issues, I suppose!).
>> > So it's a resend.
>> >
>> > On Thu, Sep 11, 2014 at 2:27 PM, Mahesh Bandewar <maheshb@google.com>
>> > wrote:
>> >>
>> >> On Thu, Sep 11, 2014 at 2:39 AM, Nikolay Aleksandrov
>> >> <nikolay@redhat.com> wrote:
>> >>> On 11/09/14 06:16, Mahesh Bandewar wrote:
>> >>>>
>> >>>> Earlier change to use usable slave array for TLB mode had an
>> >>>> additional
>> >>>> performance advantage. So extending the same logic to all other modes
>> >>>> that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
>> >>>> Also consolidating this with the earlier TLB change.
>> >>>>
>> >>>> The main idea is to build the usable slaves array in the control path
>> >>>> and use that array for slave selection during xmit operation.
>> >>>>
>> >>>> Measured performance in a setup with a bond of 4x1G NICs with 200
>> >>>> instances of netperf for the modes involved (3ad, xor, tlb)
>> >>>> cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
>> >>>>
>> >>>> Mode TPS-Before TPS-After
>> >>>>
>> >>>> 802.3ad : 468,694 493,101
>> >>>> TLB (lb=0): 392,583 392,965
>> >>>> XOR : 475,696 484,517
>> >>>>
>> >>>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> >>>> ---
>> >>>> v1:
>> >>>> (a) If bond_update_slave_arr() fails to allocate memory, it will
>> >>>> overwrite
>> >>>> the slave that need to be removed.
>> >>>> (b) Freeing of array will assign NULL (to handle bond->down to
>> >>>> bond->up
>> >>>> transition gracefully.
>> >>>> (c) Change from pr_debug() to pr_err() if bond_update_slave_arr()
>> >>>> returns
>> >>>> failure.
>> >>>> (d) XOR: bond_update_slave_arr() will consider mii-mon, arp-mon
>> >>>> cases
>> >>>> and
>> >>>> will populate the array even if these parameters are not used.
>> >>>> (e) 3AD: Should handle the ad_agg_selection_logic correctly.
>> >>>> v2:
>> >>>> (a) Removed rcu_read_{un}lock() calls from array manipulation
>> >>>> code.
>> >>>> (b) Slave link-events now refresh array for all these modes.
>> >>>> (c) Moved free-array call from bond_close() to bond_uninit().
>> >>>> v3:
>> >>>> (a) Fixed null pointer dereference.
>> >>>> (b) Removed bond->lock lockdep dependency.
>> >>>>
>> >>> Hello Mahesh,
>> >>> You should've given me time to respond, the reason I wrote this:
>> >>> "First a question, if a bond device in XOR mode is up and we enslave a
>> >>> single
>> >>> slave how would it start transmitting ? Same question, if we are
>> >>> enslaving
>> >>> a
>> >>> second device then the array will be rebuild with only the first upon
>> >>> NETDEV_UP
>> >>> (of course all this is in the case miimon is 0).
>> >>> The NETDEV_UP upon enslave happens before the slave is linked in."
>> >>> was not because I wanted you to remove the slave rebuilding from the
>> >>> NETDEV_UP/DOWN events, but because I didn't see how would a slave
>> >>> start
>> >>> transmitting in XOR mode after enslaving, and I just tested it - it
>> >>> doesn't
>> >>> since the slave array never gets rebuilt. The NETDEV_UP event is
>> >>> carried by
>> >>> the dev_open() done in bond_enslave() earlier so the
>> >>> bond_set_carrier() in
>> >>> the end isn't of much importance in most cases, simply do the
>> >>> following and
>> >>> you'll see:
>> >>> modprobe bonding mode=2
>> >>> ip set bond0 up
>> >>> ifenslave bond0 eth0
>> >>>
>> >>> Try to transmit anything and watch on the other side, you won't be
>> >>> able to
>> >>> see anything as there's no slave array. My second question was given
>> >>> all
>> >>> this, if you enslave any subsequent slaves, will it start transmitting
>> >>> ? But
>> >>> I just tested this scenario and it still doesn't as the array doesn't
>> >>> get
>> >>> built.
>> >>
>> > OK I tried that and I don't see what you have observed -
>> >
>> > Here is the log -
>> >
>> > ~# modprobe bonding mode=2
>> > ~# ip link set bond0 up
>> > [ 133.537516] New slave count=0
>> > ~# [add ip addr]
>> > ~# [add default route]
>> > ~# ifenslave bond0 eth0
>> > ~# [ 211.044852] Adding slave=eth0
>> > [ 211.047826] New slave count=1
>> > ~# ifenslave bond0 eth1
>> > [ 723.795877] Adding slave=eth0
>> > [ 723.798853] Adding slave=eth1
>> > [ 723.801824] New slave count=2
>> >
>> > I have added some instrumentation to see what is happening and
>> > following are the couple of changes (basically printfs!) -
>> >
>> >
>> > @@ -3730,13 +3736,16 @@ int bond_update_slave_arr(struct bonding
>> > *bond, struct slave *skipslave)
>> > continue;
>> > if (skipslave == slave)
>> > continue;
>> > +pr_err("Adding slave=%s\n", slave->dev->name);
>> > new_arr->arr[new_arr->count++] = slave;
>> > }
>> >
>> > old_arr = rcu_dereference_protected(bond->slave_arr,
>> > lockdep_rtnl_is_held() ||
>> >
>> > lockdep_is_held(&bond->curr_slave_lock));
>> > rcu_assign_pointer(bond->slave_arr, new_arr);
>> > +pr_err("New slave count=%d\n", new_arr->count);
>> > if (old_arr)
>> > kfree_rcu(old_arr, rcu);
>> >
>> >
>> >
>> Something is wrong here either you have some modified version or something
>> else is going on because with your patch + the pr_err()s in
>> bond_update_slave_arr() I get the following:
>> # modprobe bonding mode=2
>> # ip l set bond0 up
>> [ 47.905891] Slave count: 0
>> # ip addr add 192.168.160.2/24 dev bond0
>> # ifenslave bond0 eth1
>> [ 78.056764] bond0: Enslaving eth1 as an active interface with an up
>> link
>> [ 78.056789] IPv6: ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
>> *(no slave array messages)*
>> # ping ...
>> nothing.
>>
>> Doing a down/up cycle on the bond works as it should:
>> # ip l set bond0 down
>> # ip l set bond0 up
>> [ 686.786898] Adding slave: eth1
>> [ 686.787153] Adding slave: eth2
>> [ 686.787329] Slave count: 2
>> And now it begins to work.
>>
>> Same happens if I enslave a subsequent slave, I really don't see how your
>> arrays get rebuilt, do you have a different patch which does array
>> rebuilding in bond_enslave() ? The NETDEV_UP for the slave is generated by
>> dev_open() and it's too early to be caught as a slave by the bond's
>> notifier. I'd be curious to see the stack trace on your slave rebuilding
>> after the first enslave, could you insert a WARN_ON(1) in the
>> bond_update_slave_arr() for example and see what's triggering the slave
>> rebuild ?
>>
>> In contrast the same module without your patches:
>> # modprobe bonding mode=2
>> # ip l set bond0 up
>> # ip addr add 192.168.160.2/24 dev bond0
>> # ifenslave bond0 eth1
>> # ping ...
>> works.
>>
>> I'm doing these tests in a VM with virtio_net devices.
>> Also my default miimon is 0, if that matters.
>>
>> My tree looks like:
>> 9afec9969e3b15a8d52acb07df48423c2f941e50 bonding: Simplify the xmit
>> function for modes that use xmit_hash
>> d876c8ca5d46c3b4c201289f48011350efa545ce bonding: display xmit_hash_policy
>> for non-dynamic-tlb mode
>> 9b5a8a12737ee9ff100e87ab7fdfdec4d0999f4e net: bpf: only build
>> bpf_jit_binary_{alloc, free}() when jit selected
>>
>> >>> A few suggestions below, nothing serious though.
>> >>>
>> >>>
>> >>>> drivers/net/bonding/bond_3ad.c | 76 +++-----------------
>> >>>> drivers/net/bonding/bond_alb.c | 51 ++------------
>> >>>> drivers/net/bonding/bond_alb.h | 8 ---
>> >>>> drivers/net/bonding/bond_main.c | 152
>> >>>> ++++++++++++++++++++++++++++++++++++----
>> >>>> drivers/net/bonding/bonding.h | 8 +++
>> >>>> 5 files changed, 164 insertions(+), 131 deletions(-)
>> >>>>
>> >>>> diff --git a/drivers/net/bonding/bond_3ad.c
>> >>>> b/drivers/net/bonding/bond_3ad.c
>> >>>> index 5d27a6207384..516075f0a740 100644
>> >>>> --- a/drivers/net/bonding/bond_3ad.c
>> >>>> +++ b/drivers/net/bonding/bond_3ad.c
>> >>>> @@ -1579,6 +1579,8 @@ static void ad_agg_selection_logic(struct
>> >>>> aggregator
>> >>>> *agg)
>> >>>> __disable_port(port);
>> >>>> }
>> >>>> }
>> >>>> + if (bond_update_slave_arr(bond, NULL))
>> >>>> + pr_err("Failed to build slave-array for 3ad
>> >>>> mode.\n");
>> >>>> }
>> >>>>
>> >>>> /* if the selected aggregator is of join individuals
>> >>>> @@ -1717,6 +1719,8 @@ static void
>> >>>> ad_enable_collecting_distributing(struct
>> >>>> port *port)
>> >>>> port->actor_port_number,
>> >>>> port->aggregator->aggregator_identifier);
>> >>>> __enable_port(port);
>> >>>> + if (bond_update_slave_arr(port->slave->bond, NULL))
>> >>>> + pr_err("Failed to build slave-array for 3ad
>> >>>> mode.\n");
>> >>>> }
>> >>>> }
>> >>>>
>> >>>> @@ -1733,6 +1737,8 @@ static void
>> >>>> ad_disable_collecting_distributing(struct port *port)
>> >>>> port->actor_port_number,
>> >>>> port->aggregator->aggregator_identifier);
>> >>>> __disable_port(port);
>> >>>> + if (bond_update_slave_arr(port->slave->bond, NULL))
>> >>>> + pr_err("Failed to build slave-array for 3ad
>> >>>> mode.\n");
>> >>>> }
>> >>>> }
>> >>>>
>> >>>> @@ -2311,6 +2317,9 @@ void bond_3ad_handle_link_change(struct slave
>> >>>> *slave, char link)
>> >>>> */
>> >>>> port->sm_vars |= AD_PORT_BEGIN;
>> >>>>
>> >>>> + if (bond_update_slave_arr(slave->bond, NULL))
>> >>>> + pr_err("Failed to build slave-array for 3ad
>> >>>> mode.\n");
>> >>>> +
>> >>>> __release_state_machine_lock(port);
>> >>>> }
>> >>>>
>> >>>> @@ -2406,73 +2415,6 @@ int bond_3ad_get_active_agg_info(struct
>> >>>> bonding
>> >>>> *bond, struct ad_info *ad_info)
>> >>>> return ret;
>> >>>> }
>> >>>>
>> >>>> -int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
>> >>>> -{
>> >>>> - struct bonding *bond = netdev_priv(dev);
>> >>>> - struct slave *slave, *first_ok_slave;
>> >>>> - struct aggregator *agg;
>> >>>> - struct ad_info ad_info;
>> >>>> - struct list_head *iter;
>> >>>> - int slaves_in_agg;
>> >>>> - int slave_agg_no;
>> >>>> - int agg_id;
>> >>>> -
>> >>>> - if (__bond_3ad_get_active_agg_info(bond, &ad_info)) {
>> >>>> - netdev_dbg(dev, "__bond_3ad_get_active_agg_info
>> >>>> failed\n");
>> >>>> - goto err_free;
>> >>>> - }
>> >>>> -
>> >>>> - slaves_in_agg = ad_info.ports;
>> >>>> - agg_id = ad_info.aggregator_id;
>> >>>> -
>> >>>> - if (slaves_in_agg == 0) {
>> >>>> - netdev_dbg(dev, "active aggregator is empty\n");
>> >>>> - goto err_free;
>> >>>> - }
>> >>>> -
>> >>>> - slave_agg_no = bond_xmit_hash(bond, skb) % slaves_in_agg;
>> >>>> - first_ok_slave = NULL;
>> >>>> -
>> >>>> - bond_for_each_slave_rcu(bond, slave, iter) {
>> >>>> - agg = SLAVE_AD_INFO(slave)->port.aggregator;
>> >>>> - if (!agg || agg->aggregator_identifier != agg_id)
>> >>>> - continue;
>> >>>> -
>> >>>> - if (slave_agg_no >= 0) {
>> >>>> - if (!first_ok_slave &&
>> >>>> bond_slave_can_tx(slave))
>> >>>> - first_ok_slave = slave;
>> >>>> - slave_agg_no--;
>> >>>> - continue;
>> >>>> - }
>> >>>> -
>> >>>> - if (bond_slave_can_tx(slave)) {
>> >>>> - bond_dev_queue_xmit(bond, skb, slave->dev);
>> >>>> - goto out;
>> >>>> - }
>> >>>> - }
>> >>>> -
>> >>>> - if (slave_agg_no >= 0) {
>> >>>> - netdev_err(dev, "Couldn't find a slave to tx on for
>> >>>> aggregator ID %d\n",
>> >>>> - agg_id);
>> >>>> - goto err_free;
>> >>>> - }
>> >>>> -
>> >>>> - /* we couldn't find any suitable slave after the agg_no, so
>> >>>> use
>> >>>> the
>> >>>> - * first suitable found, if found.
>> >>>> - */
>> >>>> - if (first_ok_slave)
>> >>>> - bond_dev_queue_xmit(bond, skb, first_ok_slave->dev);
>> >>>> - else
>> >>>> - goto err_free;
>> >>>> -
>> >>>> -out:
>> >>>> - return NETDEV_TX_OK;
>> >>>> -err_free:
>> >>>> - /* no suitable interface, frame not sent */
>> >>>> - dev_kfree_skb_any(skb);
>> >>>> - goto out;
>> >>>> -}
>> >>>> -
>> >>>> int bond_3ad_lacpdu_recv(const struct sk_buff *skb, struct bonding
>> >>>> *bond,
>> >>>> struct slave *slave)
>> >>>> {
>> >>>> diff --git a/drivers/net/bonding/bond_alb.c
>> >>>> b/drivers/net/bonding/bond_alb.c
>> >>>> index 028496205f39..dbac0ceb17f6 100644
>> >>>> --- a/drivers/net/bonding/bond_alb.c
>> >>>> +++ b/drivers/net/bonding/bond_alb.c
>> >>>> @@ -200,7 +200,6 @@ static int tlb_initialize(struct bonding *bond)
>> >>>> static void tlb_deinitialize(struct bonding *bond)
>> >>>> {
>> >>>> struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> >>>> - struct tlb_up_slave *arr;
>> >>>>
>> >>>> _lock_tx_hashtbl_bh(bond);
>> >>>>
>> >>>> @@ -208,10 +207,6 @@ static void tlb_deinitialize(struct bonding
>> >>>> *bond)
>> >>>> bond_info->tx_hashtbl = NULL;
>> >>>>
>> >>>> _unlock_tx_hashtbl_bh(bond);
>> >>>> -
>> >>>> - arr = rtnl_dereference(bond_info->slave_arr);
>> >>>> - if (arr)
>> >>>> - kfree_rcu(arr, rcu);
>> >>>> }
>> >>>>
>> >>>> static long long compute_gap(struct slave *slave)
>> >>>> @@ -1409,39 +1404,9 @@ out:
>> >>>> return NETDEV_TX_OK;
>> >>>> }
>> >>>>
>> >>>> -static int bond_tlb_update_slave_arr(struct bonding *bond,
>> >>>> - struct slave *skipslave)
>> >>>> -{
>> >>>> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> >>>> - struct slave *tx_slave;
>> >>>> - struct list_head *iter;
>> >>>> - struct tlb_up_slave *new_arr, *old_arr;
>> >>>> -
>> >>>> - new_arr = kzalloc(offsetof(struct tlb_up_slave,
>> >>>> arr[bond->slave_cnt]),
>> >>>> - GFP_ATOMIC);
>> >>>> - if (!new_arr)
>> >>>> - return -ENOMEM;
>> >>>> -
>> >>>> - bond_for_each_slave(bond, tx_slave, iter) {
>> >>>> - if (!bond_slave_can_tx(tx_slave))
>> >>>> - continue;
>> >>>> - if (skipslave == tx_slave)
>> >>>> - continue;
>> >>>> - new_arr->arr[new_arr->count++] = tx_slave;
>> >>>> - }
>> >>>> -
>> >>>> - old_arr = rtnl_dereference(bond_info->slave_arr);
>> >>>> - rcu_assign_pointer(bond_info->slave_arr, new_arr);
>> >>>> - if (old_arr)
>> >>>> - kfree_rcu(old_arr, rcu);
>> >>>> -
>> >>>> - return 0;
>> >>>> -}
>> >>>> -
>> >>>> int bond_tlb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
>> >>>> {
>> >>>> struct bonding *bond = netdev_priv(bond_dev);
>> >>>> - struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> >>>> struct ethhdr *eth_data;
>> >>>> struct slave *tx_slave = NULL;
>> >>>> u32 hash_index;
>> >>>> @@ -1462,12 +1427,14 @@ int bond_tlb_xmit(struct sk_buff *skb, struct
>> >>>> net_device *bond_dev)
>> >>>>
>> >>>> hash_index &
>> >>>> 0xFF,
>> >>>>
>> >>>> skb->len);
>> >>>> } else {
>> >>>> - struct tlb_up_slave *slaves;
>> >>>> + struct bond_up_slave *slaves;
>> >>>> + unsigned int count;
>> >>>>
>> >>>> - slaves =
>> >>>> rcu_dereference(bond_info->slave_arr);
>> >>>> - if (slaves && slaves->count)
>> >>>> + slaves =
>> >>>> rcu_dereference(bond->slave_arr);
>> >>>> + count = slaves ? slaves->count : 0;
>> >>>> + if (count)
>> >>>
>> >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >>> In both of these cases (slaves & slaves->count) you could use likely()
>> >>> as in
>> >>> the case we have slaves to transmit, it'd be advantageous and will be
>> >>> hit
>> >>> every time. The cases that we get here and don't have a
>> >>> slave_arr/count are
>> >>> mostly 2:
>> >>> 1. The slaves got released between the start of xmit and this part
>> >>> 2. They were not eligible so the array got emptied
>> >>> In both of these cases we don't care about the fallout path since
>> >>> transmission isn't possible anyway.
>> >>> Also another minor thing is that usually in the cases where we want to
>> >>> fetch
>> >>> a variable only once ACCESS_ONCE() is used as a weak compiler barrier
>> >>> to
>> >>> make sure the compiler doesn't optimize out something. The others can
>> >>> correct me if I'm wrong, but I think in this case it's a good
>> >>> precaution for
>> >>> slaves->count.
>> >>> These are merely suggestions, I might be wrong.
>> >>>
>> > Will do.
>> >>>
>> >>>> tx_slave =
>> >>>> slaves->arr[hash_index
>> >>>> %
>> >>>> -
>> >>>> slaves->count];
>> >>>> +
>> >>>> count];
>> >>>> }
>> >>>> break;
>> >>>> }
>> >>>> @@ -1733,10 +1700,6 @@ void bond_alb_deinit_slave(struct bonding
>> >>>> *bond,
>> >>>> struct slave *slave)
>> >>>> rlb_clear_slave(bond, slave);
>> >>>> }
>> >>>>
>> >>>> - if (bond_is_nondyn_tlb(bond))
>> >>>> - if (bond_tlb_update_slave_arr(bond, slave))
>> >>>> - pr_err("Failed to build slave-array for TLB
>> >>>> mode.\n");
>> >>>> -
>> >>>> }
>> >>>>
>> >>>> /* Caller must hold bond lock for read */
>> >>>> @@ -1762,7 +1725,7 @@ void bond_alb_handle_link_change(struct bonding
>> >>>> *bond, struct slave *slave, char
>> >>>> }
>> >>>>
>> >>>> if (bond_is_nondyn_tlb(bond)) {
>> >>>> - if (bond_tlb_update_slave_arr(bond, NULL))
>> >>>> + if (bond_update_slave_arr(bond, NULL))
>> >>>> pr_err("Failed to build slave-array for TLB
>> >>>> mode.\n");
>> >>>> }
>> >>>> }
>> >>>> diff --git a/drivers/net/bonding/bond_alb.h
>> >>>> b/drivers/net/bonding/bond_alb.h
>> >>>> index aaeac61d03cf..5fc76c01636c 100644
>> >>>> --- a/drivers/net/bonding/bond_alb.h
>> >>>> +++ b/drivers/net/bonding/bond_alb.h
>> >>>> @@ -139,20 +139,12 @@ struct tlb_slave_info {
>> >>>> */
>> >>>> };
>> >>>>
>> >>>> -struct tlb_up_slave {
>> >>>> - unsigned int count;
>> >>>> - struct rcu_head rcu;
>> >>>> - struct slave *arr[0];
>> >>>> -};
>> >>>> -
>> >>>> struct alb_bond_info {
>> >>>> struct tlb_client_info *tx_hashtbl; /* Dynamically allocated
>> >>>> */
>> >>>> spinlock_t tx_hashtbl_lock;
>> >>>> u32 unbalanced_load;
>> >>>> int tx_rebalance_counter;
>> >>>> int lp_counter;
>> >>>> - /* -------- non-dynamic tlb mode only ---------*/
>> >>>> - struct tlb_up_slave __rcu *slave_arr; /* Up slaves */
>> >>>> /* -------- rlb parameters -------- */
>> >>>> int rlb_enabled;
>> >>>> struct rlb_client_info *rx_hashtbl; /* Receive hash table
>> >>>> */
>> >>>> diff --git a/drivers/net/bonding/bond_main.c
>> >>>> b/drivers/net/bonding/bond_main.c
>> >>>> index b43b2df9e5d1..4412c458939d 100644
>> >>>> --- a/drivers/net/bonding/bond_main.c
>> >>>> +++ b/drivers/net/bonding/bond_main.c
>> >>>> @@ -1700,6 +1700,10 @@ static int __bond_release_one(struct
>> >>>> net_device
>> >>>> *bond_dev,
>> >>>> write_unlock_bh(&bond->curr_slave_lock);
>> >>>> }
>> >>>>
>> >>>> + if (bond_mode_uses_xmit_hash(bond) &&
>> >>>> + bond_update_slave_arr(bond, slave))
>> >>>> + pr_err("Failed to build slave-array.\n");
>> >>>> +
>> >>>> netdev_info(bond_dev, "Releasing %s interface %s\n",
>> >>>> bond_is_active_slave(slave) ? "active" :
>> >>>> "backup",
>> >>>> slave_dev->name);
>> >>>> @@ -2015,6 +2019,10 @@ static void bond_miimon_commit(struct bonding
>> >>>> *bond)
>> >>>> bond_alb_handle_link_change(bond,
>> >>>> slave,
>> >>>>
>> >>>> BOND_LINK_UP);
>> >>>>
>> >>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>> >>>> + bond_update_slave_arr(bond, NULL))
>> >>>> + pr_err("Failed to build slave-array
>> >>>> for
>> >>>> XOR mode.\n");
>> >>>> +
>> >>>> if (!bond->curr_active_slave || slave ==
>> >>>> primary)
>> >>>> goto do_failover;
>> >>>>
>> >>>> @@ -2042,6 +2050,10 @@ static void bond_miimon_commit(struct bonding
>> >>>> *bond)
>> >>>> bond_alb_handle_link_change(bond,
>> >>>> slave,
>> >>>>
>> >>>> BOND_LINK_DOWN);
>> >>>>
>> >>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>> >>>> + bond_update_slave_arr(bond, NULL))
>> >>>> + pr_err("Failed to build slave-array
>> >>>> for
>> >>>> XOR mode.\n");
>> >>>> +
>> >>>> if (slave ==
>> >>>> rcu_access_pointer(bond->curr_active_slave))
>> >>>> goto do_failover;
>> >>>>
>> >>>> @@ -2505,6 +2517,9 @@ static void bond_loadbalance_arp_mon(struct
>> >>>> work_struct *work)
>> >>>>
>> >>>> if (slave_state_changed) {
>> >>>> bond_slave_state_change(bond);
>> >>>> + if (BOND_MODE(bond) == BOND_MODE_XOR &&
>> >>>> + bond_update_slave_arr(bond, NULL))
>> >>>> + pr_err("Failed to build slave-array
>> >>>> for
>> >>>> XOR mode.\n");
>> >>>> } else if (do_failover) {
>> >>>> /* the bond_select_active_slave must hold
>> >>>> RTNL
>> >>>> * and curr_slave_lock for write.
>> >>>> @@ -2899,11 +2914,23 @@ static int bond_slave_netdev_event(unsigned
>> >>>> long
>> >>>> event,
>> >>>> if (old_duplex != slave->duplex)
>> >>>>
>> >>>> bond_3ad_adapter_duplex_changed(slave);
>> >>>> }
>> >>>> + /* Refresh slave-array if applicable!
>> >>>> + * If the setuo does not use miimon or arpmon
>> >>>> (mode-specific!),
>> >>>> + * then these event will not cause the slave-array to
>> >>>> be
>> >>>> + * refreshed. This will cause xmit to use a slave
>> >>>> that is
>> >>>> not
>> >>>> + * usable. Avoid such situation by refeshing the
>> >>>> array at
>> >>>> these
>> >>>> + * events. If these (miimon/arpmon) parameters are
>> >>>> configured
>> >>>> + * then array gets refreshed twice and that should be
>> >>>> fine!
>> >>>> + */
>> >>>> + if (bond_mode_uses_xmit_hash(bond) &&
>> >>>> + bond_update_slave_arr(bond, NULL))
>> >>>> + pr_err("Failed to build slave-array for XOR
>> >>>> mode.\n");
>> >>>> break;
>> >>>> case NETDEV_DOWN:
>> >>>> - /*
>> >>>> - * ... Or is it this?
>> >>>> - */
>> >>>> + /* Refresh slave-array if applicable! */
>> >>>> + if (bond_mode_uses_xmit_hash(bond) &&
>> >>>> + bond_update_slave_arr(bond, NULL))
>> >>>> + pr_err("Failed to build slave-array for XOR
>> >>>> mode.\n");
>> >>>> break;
>> >>>> case NETDEV_CHANGEMTU:
>> >>>> /*
>> >>>> @@ -3147,6 +3174,10 @@ static int bond_open(struct net_device
>> >>>> *bond_dev)
>> >>>> bond_3ad_initiate_agg_selection(bond, 1);
>> >>>> }
>> >>>>
>> >>>> + if (bond_mode_uses_xmit_hash(bond) &&
>> >>>> + bond_update_slave_arr(bond, NULL))
>> >>>> + pr_err("Failed to build slave-array for XOR
>> >>>> mode.\n");
>> >>>> +
>> >>>> return 0;
>> >>>> }
>> >>>>
>> >>>> @@ -3654,15 +3685,106 @@ static int bond_xmit_activebackup(struct
>> >>>> sk_buff
>> >>>> *skb, struct net_device *bond_d
>> >>>> return NETDEV_TX_OK;
>> >>>> }
>> >>>>
>> >>>> -/* In bond_xmit_xor() , we determine the output device by using a
>> >>>> pre-
>> >>>> - * determined xmit_hash_policy(), If the selected device is not
>> >>>> enabled,
>> >>>> - * find the next active slave.
>> >>>> +/* Build the usable slaves array in control path for modes that use
>> >>>> xmit-hash
>> >>>> + * to determine the slave interface -
>> >>>> + * (a) BOND_MODE_8023AD
>> >>>> + * (b) BOND_MODE_XOR
>> >>>> + * (c) BOND_MODE_TLB && tlb_dynamic_lb == 0
>> >>>> */
>> >>>> -static int bond_xmit_xor(struct sk_buff *skb, struct net_device
>> >>>> *bond_dev)
>> >>>> +int bond_update_slave_arr(struct bonding *bond, struct slave
>> >>>> *skipslave)
>> >>>> {
>> >>>> - struct bonding *bond = netdev_priv(bond_dev);
>> >>>> + struct slave *slave;
>> >>>> + struct list_head *iter;
>> >>>> + struct bond_up_slave *new_arr, *old_arr;
>> >>>> + int slaves_in_agg;
>> >>>> + int agg_id = 0;
>> >>>> + int ret = 0;
>> >>>> +
>> >>>> + new_arr = kzalloc(offsetof(struct bond_up_slave,
>> >>>> arr[bond->slave_cnt]),
>> >>>> + GFP_ATOMIC);
>> >>>> + if (!new_arr) {
>> >>>> + ret = -ENOMEM;
>> >>>> + goto out;
>> >>>> + }
>> >>>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>> >>>> + struct ad_info ad_info;
>> >>>> +
>> >>>> + if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
>> >>>> + pr_debug("bond_3ad_get_active_agg_info
>> >>>> failed\n");
>> >>>> + kfree_rcu(new_arr, rcu);
>> >>>> + ret = -EINVAL;
>> >>>> + goto out;
>> >>>> + }
>> >>>> + slaves_in_agg = ad_info.ports;
>> >>>> + agg_id = ad_info.aggregator_id;
>> >>>> + }
>> >>>> + bond_for_each_slave(bond, slave, iter) {
>> >>>> + if (BOND_MODE(bond) == BOND_MODE_8023AD) {
>> >>>> + struct aggregator *agg;
>> >>>>
>> >>>> - bond_xmit_slave_id(bond, skb, bond_xmit_hash(bond, skb) %
>> >>>> bond->slave_cnt);
>> >>>> + agg = SLAVE_AD_INFO(slave)->port.aggregator;
>> >>>> + if (!agg || agg->aggregator_identifier !=
>> >>>> agg_id)
>> >>>> + continue;
>> >>>> + }
>> >>>> + if (!bond_slave_can_tx(slave))
>> >>>> + continue;
>> >>>> + if (skipslave == slave)
>> >>>> + continue;
>> >>>> + new_arr->arr[new_arr->count++] = slave;
>> >>>> + }
>> >>>> +
>> >>>> + old_arr = rcu_dereference_protected(bond->slave_arr,
>> >>>> + lockdep_rtnl_is_held() ||
>> >>>> +
>> >>>> lockdep_is_held(&bond->curr_slave_lock));
>> >>>> + rcu_assign_pointer(bond->slave_arr, new_arr);
>> >>>> + if (old_arr)
>> >>>> + kfree_rcu(old_arr, rcu);
>> >>>> +
>> >>>> +out:
>> >>>> + if (ret != 0 && skipslave) {
>> >>>> + int idx;
>> >>>> +
>> >>>> + /* Rare situation where caller has asked to skip a
>> >>>> specific
>> >>>> + * slave but allocation failed (most likely!). BTW
>> >>>> this is
>> >>>> + * only possible when the call is initiated from
>> >>>> + * __bond_release_one(). In this sitation; overwrite
>> >>>> the
>> >>>> + * skipslave entry in the array with the last entry
>> >>>> from
>> >>>> the
>> >>>> + * array to avoid a situation where the xmit path may
>> >>>> choose
>> >>>> + * this to-be-skipped slave to send a packet out.
>> >>>> + */
>> >>>> + old_arr = rtnl_dereference(bond->slave_arr);
>> >>>> + for (idx = 0; idx < old_arr->count; idx++) {
>> >>>> + if (skipslave == old_arr->arr[idx]) {
>> >>>> + old_arr->arr[idx] =
>> >>>> + old_arr->arr[old_arr->count-1];
>> >>>> + old_arr->count--;
>> >>>> + break;
>> >>>> + }
>> >>>> + }
>> >>>> + }
>> >>>> + return ret;
>> >>>> +}
>> >>>> +
>> >>>> +/* Use this Xmit function for 3AD as well as XOR modes. The current
>> >>>> + * usable slave array is formed in the control path. The xmit
>> >>>> function
>> >>>> + * just calculates hash and sends the packet out.
>> >>>> + */
>> >>>> +int bond_3ad_xor_xmit(struct sk_buff *skb, struct net_device *dev)
>> >>>> +{
>> >>>> + struct bonding *bond = netdev_priv(dev);
>> >>>> + struct slave *slave;
>> >>>> + struct bond_up_slave *slaves;
>> >>>> + unsigned int count;
>> >>>> +
>> >>>> + slaves = rcu_dereference(bond->slave_arr);
>> >>>> + count = slaves ? slaves->count : 0;
>> >>>> + if (count) {
>> >>>
>> >>> ^^^^^^^^^^^^^^
>> >>> The same comment as above applies here, too.
>> >>>
>> >>>
>> >>>> + slave = slaves->arr[bond_xmit_hash(bond, skb) %
>> >>>> count];
>> >>>> + bond_dev_queue_xmit(bond, skb, slave->dev);
>> >>>> + } else {
>> >>>> + dev_kfree_skb_any(skb);
>> >>>> + atomic_long_inc(&dev->tx_dropped);
>> >>>> + }
>> >>>>
>> >>>> return NETDEV_TX_OK;
>> >>>> }
>> >>>> @@ -3764,12 +3886,11 @@ static netdev_tx_t __bond_start_xmit(struct
>> >>>> sk_buff *skb, struct net_device *dev
>> >>>> return bond_xmit_roundrobin(skb, dev);
>> >>>> case BOND_MODE_ACTIVEBACKUP:
>> >>>> return bond_xmit_activebackup(skb, dev);
>> >>>> + case BOND_MODE_8023AD:
>> >>>> case BOND_MODE_XOR:
>> >>>> - return bond_xmit_xor(skb, dev);
>> >>>> + return bond_3ad_xor_xmit(skb, dev);
>> >>>> case BOND_MODE_BROADCAST:
>> >>>> return bond_xmit_broadcast(skb, dev);
>> >>>> - case BOND_MODE_8023AD:
>> >>>> - return bond_3ad_xmit_xor(skb, dev);
>> >>>> case BOND_MODE_ALB:
>> >>>> return bond_alb_xmit(skb, dev);
>> >>>> case BOND_MODE_TLB:
>> >>>> @@ -3947,6 +4068,7 @@ static void bond_uninit(struct net_device
>> >>>> *bond_dev)
>> >>>> struct bonding *bond = netdev_priv(bond_dev);
>> >>>> struct list_head *iter;
>> >>>> struct slave *slave;
>> >>>> + struct bond_up_slave *arr;
>> >>>>
>> >>>> bond_netpoll_cleanup(bond_dev);
>> >>>>
>> >>>> @@ -3955,6 +4077,12 @@ static void bond_uninit(struct net_device
>> >>>> *bond_dev)
>> >>>> __bond_release_one(bond_dev, slave->dev, true);
>> >>>> netdev_info(bond_dev, "Released all slaves\n");
>> >>>>
>> >>>> + arr = rtnl_dereference(bond->slave_arr);
>> >>>> + if (arr) {
>> >>>> + kfree_rcu(arr, rcu);
>> >>>> + RCU_INIT_POINTER(bond->slave_arr, NULL);
>> >>>> + }
>> >>>> +
>> >>>> list_del(&bond->bond_list);
>> >>>>
>> >>>> bond_debug_unregister(bond);
>> >>>> diff --git a/drivers/net/bonding/bonding.h
>> >>>> b/drivers/net/bonding/bonding.h
>> >>>> index 8375133dd347..78d6d3b7a780 100644
>> >>>> --- a/drivers/net/bonding/bonding.h
>> >>>> +++ b/drivers/net/bonding/bonding.h
>> >>>> @@ -177,6 +177,12 @@ struct slave {
>> >>>> struct kobject kobj;
>> >>>> };
>> >>>>
>> >>>> +struct bond_up_slave {
>> >>>> + unsigned int count;
>> >>>> + struct rcu_head rcu;
>> >>>> + struct slave *arr[0];
>> >>>> +};
>> >>>> +
>> >>>> /*
>> >>>> * Link pseudo-state only used internally by monitors
>> >>>> */
>> >>>> @@ -193,6 +199,7 @@ struct bonding {
>> >>>> struct slave __rcu *curr_active_slave;
>> >>>> struct slave __rcu *current_arp_slave;
>> >>>> struct slave __rcu *primary_slave;
>> >>>> + struct bond_up_slave __rcu *slave_arr; /* Array of usable
>> >>>> slaves
>> >>>> */
>> >>>> bool force_primary;
>> >>>> s32 slave_cnt; /* never change this value outside the
>> >>>> attach/detach wrappers */
>> >>>> int (*recv_probe)(const struct sk_buff *, struct bonding
>> >>>> *,
>> >>>> @@ -530,6 +537,7 @@ const char *bond_slave_link_status(s8 link);
>> >>>> struct bond_vlan_tag *bond_verify_device_path(struct net_device
>> >>>> *start_dev,
>> >>>> struct net_device
>> >>>> *end_dev,
>> >>>> int level);
>> >>>> +int bond_update_slave_arr(struct bonding *bond, struct slave
>> >>>> *skipslave);
>> >>>>
>> >>>> #ifdef CONFIG_PROC_FS
>> >>>> void bond_create_proc_entry(struct bonding *bond);
>> >>>>
>> >>>
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe netdev" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >
>>
>
^ permalink raw reply
* Re: [net-next v7 0/4] Refactor vxlan and l2tp to use common UDP tunnel APIs
From: Or Gerlitz @ 2014-09-15 17:46 UTC (permalink / raw)
To: Andy Zhou; +Cc: David Miller, Linux Netdev List
In-Reply-To: <1410777267-28237-1-git-send-email-azhou@nicira.com>
On Mon, Sep 15, 2014 at 1:34 PM, Andy Zhou <azhou@nicira.com> wrote:
> Andy Zhou (4):
> udp_tunnel: Seperate ipv6 functions into its own file.
> udp-tunnel: Expand UDP tunnel APIs
> vxlan: Refactor vxlan driver to make use of the common UDP tunnel
> functions.
> l2tp: Refactor l2tp core driver to make use of the common UDP tunnel
> functions
In V5's cover-letter you wrote "This patch series expend current UDP
tunnel APIs and refactoring current
UDP tunnel based protocols to make use of the new APIs. The main motivation
is to reduce code duplication." -- I wasn't fully sure to follow
what's the exact meaning of "expend" in this context? do we
have anything here that goes beyond refactoring?
>
> drivers/net/vxlan.c | 105 ++++++++--------------------------
> include/net/udp_tunnel.h | 83 ++++++++++++++++++++++++++-
> net/ipv4/udp_tunnel.c | 138 ++++++++++++++++++++++++---------------------
> net/ipv6/Makefile | 1 +
> net/ipv6/ip6_udp_tunnel.c | 105 ++++++++++++++++++++++++++++++++++
> net/l2tp/l2tp_core.c | 24 ++++----
> 6 files changed, 292 insertions(+), 164 deletions(-)
> create mode 100644 net/ipv6/ip6_udp_tunnel.c
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 net-next 1/7] net: Export inet_offloads and inet6_offloads
From: Tom Herbert @ 2014-09-15 17:32 UTC (permalink / raw)
To: Or Gerlitz; +Cc: Jerry Chu, David Miller, Linux Netdev List
In-Reply-To: <CAJ3xEMg5Q-qkr-tj0UiFP2tNVLg1P4aT2L2bZk9yWjq+V0zJLg@mail.gmail.com>
On Mon, Sep 15, 2014 at 10:15 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 6:13 PM, Tom Herbert <therbert@google.com> wrote:
>> On Mon, Sep 15, 2014 at 6:33 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> On Mon, Sep 15, 2014 at 6:07 AM, Tom Herbert <therbert@google.com> wrote:
>>>> Want to be able to call this in foo-over-udp offloads, etc.
>>>
>>> In the L2 gro case, we did dedicated helpers
>>> gro_find_receive/complete_by_type, not sure what was
>>> the exact rational there but worth checking, jerry?
>>>
>> It allows offload_base to be kept a static, but then
>> gro_find_receive_by_type can't be inlined.
>
>
> so we have two similar locations in the networking stack acting
> differently on the same/similar simple
> matter... a bit problematic maintainance wise, I would say.
Yes, these should be similar. I think we'd need to cleanup
gro_find_receive_by_type first: export udp_offload base and inline
these functions. Have skb_mac_gso_segment call this also.
^ permalink raw reply
* Re: DSA and skb->protocol
From: Alexander Duyck @ 2014-09-15 17:30 UTC (permalink / raw)
To: Andrew Lunn, f.fainelli; +Cc: seugene, Andrew Lunn, netdev
In-Reply-To: <20140914153751.GA28585@lunn.ch>
On 09/14/2014 08:37 AM, Andrew Lunn wrote:
> Hi Florian
>
> I've been debugging a WARNING when using DSA with a D-Link
> DIR665. I've had reports of the same warning with another device.
>
> WARNING: CPU: 0 PID: 2014 at net/core/dev.c:2260 skb_warn_bad_offload+0xd0/0x104()
> mv643xx_eth_port: caps=(0x0000000400014803, 0x00000000001b482b) len=1722 data_len=16 gso_size=1448 gso_type=1 gso_segs 2 ip_summed=3 encapsulation 0 features 400014801
> Modules linked in:
> CPU: 0 PID: 2014 Comm: sshd Tainted: G W 3.17.0-rc1-00007-g2f06b2c08099-dirty #228
> [<c000e2a0>] (unwind_backtrace) from [<c000bd88>] (show_stack+0x10/0x14)
> [<c000bd88>] (show_stack) from [<c0016db8>] (warn_slowpath_common+0x6c/0x8c)
> [<c0016db8>] (warn_slowpath_common) from [<c0016e08>] (warn_slowpath_fmt+0x30/0x40)
> [<c0016e08>] (warn_slowpath_fmt) from [<c04ebad4>] (skb_warn_bad_offload+0xd0/0x104)
> [<c04ebad4>] (skb_warn_bad_offload) from [<c03eea5c>] (skb_checksum_help+0x160/0x170)
> [<c03eea5c>] (skb_checksum_help) from [<c03eef40>] (dev_hard_start_xmit+0x3f8/0x4c0)
> [<c03eef40>] (dev_hard_start_xmit) from [<c04071ec>] (sch_direct_xmit+0x148/0x250)
> [<c04071ec>] (sch_direct_xmit) from [<c03ef280>] (__dev_queue_xmit+0x278/0x5f4)
> [<c03ef280>] (__dev_queue_xmit) from [<c04719b8>] (edsa_xmit+0xf8/0x2c8)
> [<c04719b8>] (edsa_xmit) from [<c03eee24>] (dev_hard_start_xmit+0x2dc/0x4c0)
> [<c03eee24>] (dev_hard_start_xmit) from [<c03ef3cc>] (__dev_queue_xmit+0x3c4/0x5f4)
> [<c03ef3cc>] (__dev_queue_xmit) from [<c0415e88>] (ip_finish_output+0x64c/0x920)
> [<c0415e88>] (ip_finish_output) from [<c0416e2c>] (ip_local_out_sk+0x34/0x38)
> [<c0416e2c>] (ip_local_out_sk) from [<c0417144>] (ip_queue_xmit+0x128/0x388)
> [<c0417144>] (ip_queue_xmit) from [<c042caa8>] (tcp_transmit_skb+0x534/0x93c)
> [<c042caa8>] (tcp_transmit_skb) from [<c042cff8>] (tcp_write_xmit+0x148/0xbf8)
> [<c042cff8>] (tcp_write_xmit) from [<c042dd7c>] (__tcp_push_pending_frames+0x30/0x9c)
> [<c042dd7c>] (__tcp_push_pending_frames) from [<c041fbc8>] (tcp_sendmsg+0xc0/0xcec)
> [<c041fbc8>] (tcp_sendmsg) from [<c0445710>] (inet_sendmsg+0x3c/0x70)
> [<c0445710>] (inet_sendmsg) from [<c03d7d5c>] (sock_aio_write+0xcc/0xec)
> [<c03d7d5c>] (sock_aio_write) from [<c00bb218>] (do_sync_write+0x7c/0xa4)
> [<c00bb218>] (do_sync_write) from [<c00bbc40>] (vfs_write+0x108/0x1b0)
> [<c00bbc40>] (vfs_write) from [<c00bc214>] (SyS_write+0x40/0x94)
> [<c00bc214>] (SyS_write) from [<c0009480>] (ret_fast_syscall+0x0/0x2c)
> ---[ end trace b1b02d15aba4766a ]---
>
> I think i understand what is going on, and one of your recent patches
> may indicate you have seen something similar.
>
> int dev_hard_start_xmit() we have:
>
> 2607
> 2608 features = netif_skb_features(skb);
>
> ...
> 2637 /* If packet is not checksummed and device does not
> 2638 * support checksumming for this protocol, complete
> 2639 * checksumming here.
> 2640 */
> 2641 if (skb->ip_summed == CHECKSUM_PARTIAL) {
> 2642 if (skb->encapsulation)
> 2643 skb_set_inner_transport_header(skb,
> 2644 skb_checksum_start_offset(skb));
> 2645 else
> 2646 skb_set_transport_header(skb,
> 2647 skb_checksum_start_offset(skb));
> 2648 if (!(features & NETIF_F_ALL_CSUM) &&
> 2649 skb_checksum_help(skb))
> 2650 goto out_kfree_skb;
> 2651 }
>
> This packet is CHECKSUM_PARTIAL, but it is also a GSO packet, with two
> segments, so the skb_checksum_help() is throwing the warning. It is
> expected that the hardware with do the checksum when using GSO. The
> reason it is trying to do software checksums, not hardware, is because
> features does not indicate the protocol is supported by hardware
> checksumming. This i initially thought was odd, since this is a TCP/IP
> packet, as you can see from the stack trace. However,
>
> netif_skb_features(skb) calls harmonize_features() which calls
> can_checksum_protocol():
>
> 3206 static inline bool can_checksum_protocol(netdev_features_t features,
> 3207 __be16 protocol)
> 3208 {
> 3209 return ((features & NETIF_F_GEN_CSUM) ||
> 3210 ((features & NETIF_F_V4_CSUM) &&
> 3211 protocol == htons(ETH_P_IP)) ||
> 3212 ((features & NETIF_F_V6_CSUM) &&
> 3213 protocol == htons(ETH_P_IPV6)) ||
> 3214 ((features & NETIF_F_FCOE_CRC) &&
> 3215 protocol == htons(ETH_P_FCOE)));
> 3216 }
>
> However looking in the skb, we see protocol is now ETH_P_EDSA, not
> ETH_P_IP. Hence the hardware does not support this protocol.
>
> This protocol value is because edsa_xmit() changes it in the slave
> device TX path.
>
> Your patch "net: dsa: change tag_protocol to an enum"
> has a hunk:
>
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index e0b759ec209e..8fbc21c0de78 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -91,7 +91,6 @@ static netdev_tx_t brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev)
> /* Queue the SKB for transmission on the parent interface, but
> * do not modify its EtherType
> */
> - skb->protocol = htons(ETH_P_BRCMTAG);
> skb->dev = p->parent->dst->master_netdev;
> dev_queue_xmit(skb);
>
> making me think it is not actually needed to change
> skb->protocol. When i remove this from edsa_xmit(), the warning goes
> away and networking seems to work O.K.
>
> Is this the right fix? Should we remove this setting of skb->protocol
> in the other dsa xmit functions?
>
> Thanks
> Andrew
>
No, if anything they should all be using ETH_P_XDSA to indicate the
protocol between the switch and the host network interface. When you
triggered this issue did you by any chance change some of the settings
on the host netdev for the switch?
>From what I can tell it looks like the issue might be related to the
fact that the slave features are based off of the vlan_features for the
host device. If for example the vlan_features on your host device were
recently changed that could be one cause for this issue.
The fix would probably be to update skb_network_protocol so that it can
sort out ETH_P_XDSA tags and get the Ethertype from the frame.
Thanks,
Alex
^ permalink raw reply
* Re: Qdisc: Measuring Head-of-Line blocking with netperf-wrapper
From: Eric Dumazet @ 2014-09-15 17:24 UTC (permalink / raw)
To: Tom Herbert
Cc: Jesper Dangaard Brouer, netdev@vger.kernel.org, Stephen Hemminger,
David Miller, Hannes Frederic Sowa, Daniel Borkmann,
Florian Westphal, Toke Høiland-Jørgensen, Dave Taht
In-Reply-To: <CA+mtBx_EthN8G0oEUENmVhb8MJ2MrijhT+FRjJPomBLvJPoH_w@mail.gmail.com>
On Mon, 2014-09-15 at 10:10 -0700, Tom Herbert wrote:
> On Mon, Sep 15, 2014 at 9:45 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> >
> > Hi Eric,
> >
> > I've constructed a "netperf-wrapper" test for measuring Head-of-Line
> > blocking, called "tcp_upload_prio", that I hope you will approve of?
> >
> > https://github.com/tohojo/netperf-wrapper/commit/1e6b755e8051b6
> >
> > The basic idea is to have ping packets with TOS bit 0x10, which end-up
> > in the high-prio band of pfifo_fast. While two TCP uploads utilize
> > all the bandwidth.
> >
> > These high-prio ping packet should then demonstrate the Head-of-Line
> > blocking occurring due to 1) packets in the HW TX ring buffer, or
> > 2) in the qdisc layers requeue mechanism. Disgusting these two case
> > might be a little difficult.
> >
> >
> >
> > Special care need to be take for using this on the default
> > qdisc MQ which have pfifo_fast assigned for every HW queue.
> >
> > Setup requirements:
> > 1. IRQ align CPUs to NIC HW queues
> > 2. Force netperf-wrapper subcommands to run the same CPU
> > E.g: taskset -c 2 ./netperf-wrapper -H IP tcp_upload_prio
> >
> > This will force all measurements to go through the same qdisc. This
> > is needed so the ping/latency tests measures the real property of
> > the qdisc and Head-of-Line blocking effect.
> >
> >
> > Basically the same as:
> > sudo taskset -c 2 ping -Q 0x10 192.168.8.2
> > sudo taskset -c 2 ping 192.168.8.2
> > sudo taskset -c 2 netperf -H 192.168.8.2 -t TCP_STREAM -l 120
> > sudo taskset -c 2 netperf -H 192.168.8.2 -t TCP_STREAM -l 120
> > --
> ping is a very coarse way to measure latency and in network devices it
> doesn't follow same path as TCP/UDP (no 4-tuple for RSS, ECMP) so it's
> biased and not a very realistic workload. You might want to try using
> netperf TCP_RR at higher priority for a fairer comparison (this is
> what I used to verify BQL benefits). Also, you probably want to make
> sure to have enough antagonist flows to saturate all links when using
> MQ.
Right.
Jesper, relevant netperf option is :
-y local,remote Set the socket priority
^ permalink raw reply
* Re: [PATCH net-next v2 1/4] openvswitch: refactor ovs flow extract API.
From: Pravin Shelar @ 2014-09-15 17:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20140913.164708.1690675945507354047.davem@davemloft.net>
On Sat, Sep 13, 2014 at 1:47 PM, David Miller <davem@davemloft.net> wrote:
> From: Pravin B Shelar <pshelar@nicira.com>
> Date: Thu, 11 Sep 2014 16:28:04 -0700
>
>> @@ -250,7 +251,7 @@ void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb)
>> stats = this_cpu_ptr(dp->stats_percpu);
>>
>> /* Extract flow from 'skb' into 'key'. */
>> - error = ovs_flow_extract(skb, p->port_no, &key);
>> + error = ovs_flow_key_extract(skb, &key);
>> if (unlikely(error)) {
>> kfree_skb(skb);
>> return;
> ...
>> @@ -611,5 +604,35 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>> }
>> }
>>
>> + OVS_CB(skb)->pkt_key = key;
>> return 0;
>
> Wow, have you really been putting pointers to kernel stack variables
> into the SKB control block all this time?
>
> That's error prone as well as asking for trouble.
>
Good point.
> Please adjust the code to not do this.
But this is significantly different than the patch series. If I change
this patch, I will need to change all patches in this series. So I
would add one patch at the end of this series to fix this issue.
Thanks,
Pravin.
^ permalink raw reply
* Re: [PATCH v2 net-next 3/7] fou: Add GRO support
From: Tom Herbert @ 2014-09-15 17:21 UTC (permalink / raw)
To: Or Gerlitz; +Cc: David Miller, Linux Netdev List
In-Reply-To: <CAJ3xEMh-76WMatuknjywQK=ZVWG8UM5swEC0CE5FTv-ts-Jk6g@mail.gmail.com>
On Mon, Sep 15, 2014 at 10:03 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Mon, Sep 15, 2014 at 6:10 PM, Tom Herbert <therbert@google.com> wrote:
>> On Mon, Sep 15, 2014 at 8:00 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>>> On Mon, Sep 15, 2014 at 6:07 AM, Tom Herbert <therbert@google.com> wrote:
>>>> Implement fou_gro_receive and fou_gro_complete, and populate these
>>>> in the correponsing udp_offloads for the socket. Added ipproto to
>>>> udp_offloads and pass this from UDP to the fou GRO routine in proto
>>>> field of napi_gro_cb structure.
>>>
>>>
>>> Do we really need that extra hop of fou4_gro_receive/complete?
>>> can't we somehow plant the gro receive/complete (say) GRE handlers in
>>> the udp offload
>>> struct with the UDP port that related to (say) GRE over UDP tunneling?
>>>
>> That would be nice, but it isn't obvious to me how to manage the
>> references. The offload functions are accessed with RCU pretty consistently.
>
> Currently udp_gro_receive calls rcu_read_lock() before it invokes
> fou4/6_gro_receive
> and rcu_read_unlock after the fou calls returns. The Fou call repeats
> the same practice
> w.r.t the (say) GRE gro receive callback, so... what happens if we
> eliminate the fou part all together?
Yes, we could conceivably index into inet_offloads directly from
udp_gro_receive in lieu of calling the offload functions in the
structure. It is more special case code in udp though and I'm not sure
that makes it a win.
^ permalink raw reply
* Re: [PATCH v2 net-next 1/7] net: Export inet_offloads and inet6_offloads
From: Or Gerlitz @ 2014-09-15 17:15 UTC (permalink / raw)
To: Tom Herbert; +Cc: Jerry Chu, David Miller, Linux Netdev List
In-Reply-To: <CA+mtBx8ZacxVFEB-odM4qrbawFTWUsijTMFGYBWiLV4Ad22ctw@mail.gmail.com>
On Mon, Sep 15, 2014 at 6:13 PM, Tom Herbert <therbert@google.com> wrote:
> On Mon, Sep 15, 2014 at 6:33 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Mon, Sep 15, 2014 at 6:07 AM, Tom Herbert <therbert@google.com> wrote:
>>> Want to be able to call this in foo-over-udp offloads, etc.
>>
>> In the L2 gro case, we did dedicated helpers
>> gro_find_receive/complete_by_type, not sure what was
>> the exact rational there but worth checking, jerry?
>>
> It allows offload_base to be kept a static, but then
> gro_find_receive_by_type can't be inlined.
so we have two similar locations in the networking stack acting
differently on the same/similar simple
matter... a bit problematic maintainance wise, I would say.
^ permalink raw reply
* [Patch net-next 3/4] net:fec: increase DMA queue number
From: Frank.Li @ 2014-09-15 17:12 UTC (permalink / raw)
To: b38611, davem, netdev, lznuaa
Cc: Frank Li, shawn.guo, Fugang Duan, linux-arm-kernel
In-Reply-To: <1410801177-15872-1-git-send-email-Frank.Li@freescale.com>
From: Fugang Duan <B38611@freescale.com>
when enable interrupt coalesce, 8 BD is not enough.
Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
drivers/net/ethernet/freescale/fec.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index f13e319..c61eff6 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -296,7 +296,7 @@ struct bufdesc_ex {
* the skbuffer directly.
*/
-#define FEC_ENET_RX_PAGES 8
+#define FEC_ENET_RX_PAGES 256
#define FEC_ENET_RX_FRSIZE 2048
#define FEC_ENET_RX_FRPPG (PAGE_SIZE / FEC_ENET_RX_FRSIZE)
#define RX_RING_SIZE (FEC_ENET_RX_FRPPG * FEC_ENET_RX_PAGES)
--
1.9.1
^ permalink raw reply related
* Re: Qdisc: Measuring Head-of-Line blocking with netperf-wrapper
From: Tom Herbert @ 2014-09-15 17:10 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Eric Dumazet, netdev@vger.kernel.org, Stephen Hemminger,
David Miller, Hannes Frederic Sowa, Daniel Borkmann,
Florian Westphal, Toke Høiland-Jørgensen, Dave Taht
In-Reply-To: <20140915184517.6c5474e5@redhat.com>
On Mon, Sep 15, 2014 at 9:45 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> Hi Eric,
>
> I've constructed a "netperf-wrapper" test for measuring Head-of-Line
> blocking, called "tcp_upload_prio", that I hope you will approve of?
>
> https://github.com/tohojo/netperf-wrapper/commit/1e6b755e8051b6
>
> The basic idea is to have ping packets with TOS bit 0x10, which end-up
> in the high-prio band of pfifo_fast. While two TCP uploads utilize
> all the bandwidth.
>
> These high-prio ping packet should then demonstrate the Head-of-Line
> blocking occurring due to 1) packets in the HW TX ring buffer, or
> 2) in the qdisc layers requeue mechanism. Disgusting these two case
> might be a little difficult.
>
>
>
> Special care need to be take for using this on the default
> qdisc MQ which have pfifo_fast assigned for every HW queue.
>
> Setup requirements:
> 1. IRQ align CPUs to NIC HW queues
> 2. Force netperf-wrapper subcommands to run the same CPU
> E.g: taskset -c 2 ./netperf-wrapper -H IP tcp_upload_prio
>
> This will force all measurements to go through the same qdisc. This
> is needed so the ping/latency tests measures the real property of
> the qdisc and Head-of-Line blocking effect.
>
>
> Basically the same as:
> sudo taskset -c 2 ping -Q 0x10 192.168.8.2
> sudo taskset -c 2 ping 192.168.8.2
> sudo taskset -c 2 netperf -H 192.168.8.2 -t TCP_STREAM -l 120
> sudo taskset -c 2 netperf -H 192.168.8.2 -t TCP_STREAM -l 120
> --
ping is a very coarse way to measure latency and in network devices it
doesn't follow same path as TCP/UDP (no 4-tuple for RSS, ECMP) so it's
biased and not a very realistic workload. You might want to try using
netperf TCP_RR at higher priority for a fairer comparison (this is
what I used to verify BQL benefits). Also, you probably want to make
sure to have enough antagonist flows to saturate all links when using
MQ.
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Sr. Network Kernel Developer at Red Hat
> Author of http://www.iptv-analyzer.org
> LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH 2/2] dsa: Replace mii_bus with a generic host device
From: Alexander Duyck @ 2014-09-15 17:00 UTC (permalink / raw)
To: netdev; +Cc: f.fainelli, kernel, davem
In-Reply-To: <20140915165543.1261.2520.stgit@ahduyck-bv4.jf.intel.com>
This change makes it so that instead of passing and storing a mii_bus we
instead pass and store a host_dev. From there we can test to determine the
exact type of device, and can verify it is the correct device for our switch.
So for example it would be possible to pass a device pointer from a pci_dev
and instead of checking for a PHY ID we could check for a vendor and/or device
ID.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
arch/arm/plat-orion/common.c | 2 +-
drivers/net/dsa/bcm_sf2.c | 2 +-
drivers/net/dsa/mv88e6060.c | 13 +++++++++----
drivers/net/dsa/mv88e6123_61_65.c | 6 +++++-
drivers/net/dsa/mv88e6131.c | 6 +++++-
drivers/net/dsa/mv88e6171.c | 6 +++++-
drivers/net/dsa/mv88e6xxx.c | 4 ++--
include/net/dsa.h | 9 +++++----
net/dsa/dsa.c | 24 ++++++++----------------
net/dsa/slave.c | 2 +-
10 files changed, 42 insertions(+), 32 deletions(-)
diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c
index 3ec6e8e..f5b00f4 100644
--- a/arch/arm/plat-orion/common.c
+++ b/arch/arm/plat-orion/common.c
@@ -499,7 +499,7 @@ void __init orion_ge00_switch_init(struct dsa_platform_data *d, int irq)
d->netdev = &orion_ge00.dev;
for (i = 0; i < d->nr_chips; i++)
- d->chip[i].mii_bus = &orion_ge00_shared.dev;
+ d->chip[i].host_dev = &orion_ge00_shared.dev;
orion_switch_device.dev.platform_data = d;
platform_device_register(&orion_switch_device);
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index e9918c7..02d7db3 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -129,7 +129,7 @@ static int bcm_sf2_sw_get_sset_count(struct dsa_switch *ds)
return BCM_SF2_STATS_SIZE;
}
-static char *bcm_sf2_sw_probe(struct mii_bus *bus, int sw_addr)
+static char *bcm_sf2_sw_probe(struct device *host_dev, int sw_addr)
{
return "Broadcom Starfighter 2";
}
diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index d8037c1..776e965 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -21,7 +21,8 @@
static int reg_read(struct dsa_switch *ds, int addr, int reg)
{
- return mdiobus_read(ds->master_mii_bus, ds->pd->sw_addr + addr, reg);
+ return mdiobus_read(to_mii_bus(ds->master_dev),
+ ds->pd->sw_addr + addr, reg);
}
#define REG_READ(addr, reg) \
@@ -37,8 +38,8 @@ static int reg_read(struct dsa_switch *ds, int addr, int reg)
static int reg_write(struct dsa_switch *ds, int addr, int reg, u16 val)
{
- return mdiobus_write(ds->master_mii_bus, ds->pd->sw_addr + addr,
- reg, val);
+ return mdiobus_write(to_mii_bus(ds->master_dev),
+ ds->pd->sw_addr + addr, reg, val);
}
#define REG_WRITE(addr, reg, val) \
@@ -50,10 +51,14 @@ static int reg_write(struct dsa_switch *ds, int addr, int reg, u16 val)
return __ret; \
})
-static char *mv88e6060_probe(struct mii_bus *bus, int sw_addr)
+static char *mv88e6060_probe(struct device *host_dev, int sw_addr)
{
+ struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);
int ret;
+ if (bus == NULL)
+ return NULL;
+
ret = mdiobus_read(bus, sw_addr + REG_PORT(0), 0x03);
if (ret >= 0) {
ret &= 0xfff0;
diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
index 975774f..a332c53 100644
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ b/drivers/net/dsa/mv88e6123_61_65.c
@@ -17,10 +17,14 @@
#include <net/dsa.h>
#include "mv88e6xxx.h"
-static char *mv88e6123_61_65_probe(struct mii_bus *bus, int sw_addr)
+static char *mv88e6123_61_65_probe(struct device *host_dev, int sw_addr)
{
+ struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);
int ret;
+ if (bus == NULL)
+ return NULL;
+
ret = __mv88e6xxx_reg_read(bus, sw_addr, REG_PORT(0), 0x03);
if (ret >= 0) {
if (ret == 0x1212)
diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
index 35541f2..244c735 100644
--- a/drivers/net/dsa/mv88e6131.c
+++ b/drivers/net/dsa/mv88e6131.c
@@ -22,10 +22,14 @@
#define ID_6095 0x0950
#define ID_6131 0x1060
-static char *mv88e6131_probe(struct mii_bus *bus, int sw_addr)
+static char *mv88e6131_probe(struct device *host_dev, int sw_addr)
{
+ struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);
int ret;
+ if (bus == NULL)
+ return NULL;
+
ret = __mv88e6xxx_reg_read(bus, sw_addr, REG_PORT(0), 0x03);
if (ret >= 0) {
ret &= 0xfff0;
diff --git a/drivers/net/dsa/mv88e6171.c b/drivers/net/dsa/mv88e6171.c
index 03a7006..6365e30 100644
--- a/drivers/net/dsa/mv88e6171.c
+++ b/drivers/net/dsa/mv88e6171.c
@@ -17,10 +17,14 @@
#include <net/dsa.h>
#include "mv88e6xxx.h"
-static char *mv88e6171_probe(struct mii_bus *bus, int sw_addr)
+static char *mv88e6171_probe(struct device *host_dev, int sw_addr)
{
+ struct mii_bus *bus = dsa_host_dev_to_mii_bus(host_dev);
int ret;
+ if (bus == NULL)
+ return NULL;
+
ret = __mv88e6xxx_reg_read(bus, sw_addr, REG_PORT(0), 0x03);
if (ret >= 0) {
if ((ret & 0xfff0) == 0x1710)
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 901d2a9..d6f6428 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -78,7 +78,7 @@ int mv88e6xxx_reg_read(struct dsa_switch *ds, int addr, int reg)
int ret;
mutex_lock(&ps->smi_mutex);
- ret = __mv88e6xxx_reg_read(ds->master_mii_bus,
+ ret = __mv88e6xxx_reg_read(to_mii_bus(ds->master_dev),
ds->pd->sw_addr, addr, reg);
mutex_unlock(&ps->smi_mutex);
@@ -122,7 +122,7 @@ int mv88e6xxx_reg_write(struct dsa_switch *ds, int addr, int reg, u16 val)
int ret;
mutex_lock(&ps->smi_mutex);
- ret = __mv88e6xxx_reg_write(ds->master_mii_bus,
+ ret = __mv88e6xxx_reg_write(to_mii_bus(ds->master_dev),
ds->pd->sw_addr, addr, reg, val);
mutex_unlock(&ps->smi_mutex);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index a55c4e6..c779e9b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -34,7 +34,7 @@ struct dsa_chip_data {
/*
* How to access the switch configuration registers.
*/
- struct device *mii_bus;
+ struct device *host_dev;
int sw_addr;
/* Device tree node pointer for this specific switch chip
@@ -134,9 +134,9 @@ struct dsa_switch {
struct dsa_switch_driver *drv;
/*
- * Reference to mii bus to use.
+ * Reference to host device to use.
*/
- struct mii_bus *master_mii_bus;
+ struct device *master_dev;
/*
* Slave mii_bus and devices for the individual ports.
@@ -178,7 +178,7 @@ struct dsa_switch_driver {
/*
* Probing and setup.
*/
- char *(*probe)(struct mii_bus *bus, int sw_addr);
+ char *(*probe)(struct device *host_dev, int sw_addr);
int (*setup)(struct dsa_switch *ds);
int (*set_addr)(struct dsa_switch *ds, u8 *addr);
@@ -213,6 +213,7 @@ struct dsa_switch_driver {
void register_switch_driver(struct dsa_switch_driver *type);
void unregister_switch_driver(struct dsa_switch_driver *type);
+struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev);
static inline void *ds_to_priv(struct dsa_switch *ds)
{
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 1df0a7c..b34d697 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -43,7 +43,7 @@ void unregister_switch_driver(struct dsa_switch_driver *drv)
EXPORT_SYMBOL_GPL(unregister_switch_driver);
static struct dsa_switch_driver *
-dsa_switch_probe(struct mii_bus *bus, int sw_addr, char **_name)
+dsa_switch_probe(struct device *host_dev, int sw_addr, char **_name)
{
struct dsa_switch_driver *ret;
struct list_head *list;
@@ -58,7 +58,7 @@ dsa_switch_probe(struct mii_bus *bus, int sw_addr, char **_name)
drv = list_entry(list, struct dsa_switch_driver, list);
- name = drv->probe(bus, sw_addr);
+ name = drv->probe(host_dev, sw_addr);
if (name != NULL) {
ret = drv;
break;
@@ -75,7 +75,7 @@ dsa_switch_probe(struct mii_bus *bus, int sw_addr, char **_name)
/* basic switch operations **************************************************/
static struct dsa_switch *
dsa_switch_setup(struct dsa_switch_tree *dst, int index,
- struct device *parent, struct mii_bus *bus)
+ struct device *parent, struct device *host_dev)
{
struct dsa_chip_data *pd = dst->pd->chip + index;
struct dsa_switch_driver *drv;
@@ -88,7 +88,7 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
/*
* Probe for switch model.
*/
- drv = dsa_switch_probe(bus, pd->sw_addr, &name);
+ drv = dsa_switch_probe(host_dev, pd->sw_addr, &name);
if (drv == NULL) {
printk(KERN_ERR "%s[%d]: could not detect attached switch\n",
dst->master_netdev->name, index);
@@ -109,8 +109,7 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
ds->index = index;
ds->pd = dst->pd->chip + index;
ds->drv = drv;
- ds->master_mii_bus = bus;
-
+ ds->master_dev = host_dev;
/*
* Validate supplied switch configuration.
@@ -285,7 +284,7 @@ static struct device *dev_find_class(struct device *parent, char *class)
return device_find_child(parent, class, dev_is_class);
}
-static struct mii_bus *dev_to_mii_bus(struct device *dev)
+struct mii_bus *dsa_host_dev_to_mii_bus(struct device *dev)
{
struct device *d;
@@ -301,6 +300,7 @@ static struct mii_bus *dev_to_mii_bus(struct device *dev)
return NULL;
}
+EXPORT_SYMBOL_GPL(dsa_host_dev_to_mii_bus);
static struct net_device *dev_to_net_device(struct device *dev)
{
@@ -566,17 +566,9 @@ static int dsa_probe(struct platform_device *pdev)
dst->cpu_port = -1;
for (i = 0; i < pd->nr_chips; i++) {
- struct mii_bus *bus;
struct dsa_switch *ds;
- bus = dev_to_mii_bus(pd->chip[i].mii_bus);
- if (bus == NULL) {
- printk(KERN_ERR "%s[%d]: no mii bus found for "
- "dsa switch\n", dev->name, i);
- continue;
- }
-
- ds = dsa_switch_setup(dst, i, &pdev->dev, bus);
+ ds = dsa_switch_setup(dst, i, &pdev->dev, pd->chip[i].host_dev);
if (IS_ERR(ds)) {
printk(KERN_ERR "%s[%d]: couldn't create dsa switch "
"instance (error %ld)\n", dev->name, i,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index e38a331..90c9689 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -44,7 +44,7 @@ void dsa_slave_mii_bus_init(struct dsa_switch *ds)
ds->slave_mii_bus->write = dsa_slave_phy_write;
snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "dsa-%d:%.2x",
ds->index, ds->pd->sw_addr);
- ds->slave_mii_bus->parent = &ds->master_mii_bus->dev;
+ ds->slave_mii_bus->parent = ds->master_dev;
}
^ permalink raw reply related
* [PATCH 1/2] dsa: Split ops up, and avoid assigning tag_protocol and receive separately
From: Alexander Duyck @ 2014-09-15 17:00 UTC (permalink / raw)
To: netdev; +Cc: f.fainelli, kernel, davem
In-Reply-To: <20140915165543.1261.2520.stgit@ahduyck-bv4.jf.intel.com>
This change addresses several issues.
First, it was possible to set tag_protocol without setting the ops pointer.
To correct that I have reordered things so that rcv is now populated before
we set tag_protocol.
Second, it didn't make much sense to keep setting the device ops each time a
new slave was registered. So by moving the receive portion out into root
switch initialization that issue should be addressed.
Third, I wanted to avoid sending tags if the rcv pointer was not registered
so I changed the tag check to verify if the rcv function pointer is set on
the root tree. If it is then we start sending DSA tagged frames.
Finally I split the device ops pointer in the structures into two spots. I
placed the rcv function pointer in the root switch since this makes it
easiest to access from there, and I placed the xmit function pointer in the
slave for the same reason.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
include/linux/netdevice.h | 7 -------
include/net/dsa.h | 10 ++++++----
net/dsa/dsa.c | 32 ++++++++++++++++++++++++++++----
net/dsa/dsa_priv.h | 11 ++++++++++-
net/dsa/slave.c | 37 +++++++++++++++----------------------
net/dsa/tag_brcm.c | 1 -
net/dsa/tag_dsa.c | 1 -
net/dsa/tag_edsa.c | 1 -
net/dsa/tag_trailer.c | 1 -
9 files changed, 59 insertions(+), 42 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f9e81d1..28d4378 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1928,13 +1928,6 @@ struct udp_offload {
struct offload_callbacks callbacks;
};
-struct dsa_device_ops {
- netdev_tx_t (*xmit)(struct sk_buff *skb, struct net_device *dev);
- int (*rcv)(struct sk_buff *skb, struct net_device *dev,
- struct packet_type *pt, struct net_device *orig_dev);
-};
-
-
/* often modified stats are per cpu, other are shared (netdev->stats) */
struct pcpu_sw_netstats {
u64 rx_packets;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8a8a5d9..a55c4e6 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -77,7 +77,7 @@ struct dsa_platform_data {
struct dsa_chip_data *chip;
};
-struct dsa_device_ops;
+struct packet_type;
struct dsa_switch_tree {
/*
@@ -91,7 +91,10 @@ struct dsa_switch_tree {
* protocol to use.
*/
struct net_device *master_netdev;
- const struct dsa_device_ops *ops;
+ int (*rcv)(struct sk_buff *skb,
+ struct net_device *dev,
+ struct packet_type *pt,
+ struct net_device *orig_dev);
enum dsa_tag_protocol tag_protocol;
/*
@@ -218,7 +221,6 @@ static inline void *ds_to_priv(struct dsa_switch *ds)
static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
{
- return dst->tag_protocol != DSA_TAG_PROTO_NONE;
+ return dst->rcv != NULL;
}
-
#endif
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 61f145c..1df0a7c 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -10,7 +10,6 @@
*/
#include <linux/list.h>
-#include <linux/netdevice.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/module.h>
@@ -154,9 +153,34 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
* tagging protocol to the preferred tagging format of this
* switch.
*/
- if (ds->dst->cpu_switch == index)
- ds->dst->tag_protocol = drv->tag_protocol;
+ if (dst->cpu_switch == index) {
+ switch (drv->tag_protocol) {
+#ifdef CONFIG_NET_DSA_TAG_DSA
+ case DSA_TAG_PROTO_DSA:
+ dst->rcv = dsa_netdev_ops.rcv;
+ break;
+#endif
+#ifdef CONFIG_NET_DSA_TAG_EDSA
+ case DSA_TAG_PROTO_EDSA:
+ dst->rcv = edsa_netdev_ops.rcv;
+ break;
+#endif
+#ifdef CONFIG_NET_DSA_TAG_TRAILER
+ case DSA_TAG_PROTO_TRAILER:
+ dst->rcv = trailer_netdev_ops.rcv;
+ break;
+#endif
+#ifdef CONFIG_NET_DSA_TAG_BRCM
+ case DSA_TAG_PROTO_BRCM:
+ dst->rcv = brcm_netdev_ops.rcv;
+ break;
+#endif
+ default:
+ break;
+ }
+ dst->tag_protocol = drv->tag_protocol;
+ }
/*
* Do basic register setup.
@@ -626,7 +650,7 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
return 0;
}
- return dst->ops->rcv(skb, dev, pt, orig_dev);
+ return dst->rcv(skb, dev, pt, orig_dev);
}
static struct packet_type dsa_pack_type __read_mostly = {
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 98afed4..f90899e 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -12,7 +12,13 @@
#define __DSA_PRIV_H
#include <linux/phy.h>
-#include <net/dsa.h>
+#include <linux/netdevice.h>
+
+struct dsa_device_ops {
+ netdev_tx_t (*xmit)(struct sk_buff *skb, struct net_device *dev);
+ int (*rcv)(struct sk_buff *skb, struct net_device *dev,
+ struct packet_type *pt, struct net_device *orig_dev);
+};
struct dsa_slave_priv {
/*
@@ -20,6 +26,8 @@ struct dsa_slave_priv {
* switch port.
*/
struct net_device *dev;
+ netdev_tx_t (*xmit)(struct sk_buff *skb,
+ struct net_device *dev);
/*
* Which switch this port is a part of, and the port index
@@ -43,6 +51,7 @@ struct dsa_slave_priv {
extern char dsa_driver_version[];
/* slave.c */
+extern const struct dsa_device_ops notag_netdev_ops;
void dsa_slave_mii_bus_init(struct dsa_switch *ds);
struct net_device *dsa_slave_create(struct dsa_switch *ds,
struct device *parent,
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 809eeb1..e38a331 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -9,7 +9,6 @@
*/
#include <linux/list.h>
-#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/phy.h>
#include <linux/of_net.h>
@@ -176,9 +175,8 @@ static int dsa_slave_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct dsa_slave_priv *p = netdev_priv(dev);
- struct dsa_switch_tree *dst = p->parent->dst;
- return dst->ops->xmit(skb, dev);
+ return p->xmit(skb, dev);
}
static netdev_tx_t dsa_slave_notag_xmit(struct sk_buff *skb,
@@ -325,11 +323,6 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
.ndo_do_ioctl = dsa_slave_ioctl,
};
-static const struct dsa_device_ops notag_netdev_ops = {
- .xmit = dsa_slave_notag_xmit,
- .rcv = NULL,
-};
-
static void dsa_slave_adjust_link(struct net_device *dev)
{
struct dsa_slave_priv *p = netdev_priv(dev);
@@ -435,41 +428,41 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent,
slave_dev->tx_queue_len = 0;
slave_dev->netdev_ops = &dsa_slave_netdev_ops;
+ SET_NETDEV_DEV(slave_dev, parent);
+ slave_dev->dev.of_node = ds->pd->port_dn[port];
+ slave_dev->vlan_features = master->vlan_features;
+
+ p = netdev_priv(slave_dev);
+ p->dev = slave_dev;
+ p->parent = ds;
+ p->port = port;
+
switch (ds->dst->tag_protocol) {
#ifdef CONFIG_NET_DSA_TAG_DSA
case DSA_TAG_PROTO_DSA:
- ds->dst->ops = &dsa_netdev_ops;
+ p->xmit = dsa_netdev_ops.xmit;
break;
#endif
#ifdef CONFIG_NET_DSA_TAG_EDSA
case DSA_TAG_PROTO_EDSA:
- ds->dst->ops = &edsa_netdev_ops;
+ p->xmit = edsa_netdev_ops.xmit;
break;
#endif
#ifdef CONFIG_NET_DSA_TAG_TRAILER
case DSA_TAG_PROTO_TRAILER:
- ds->dst->ops = &trailer_netdev_ops;
+ p->xmit = trailer_netdev_ops.xmit;
break;
#endif
#ifdef CONFIG_NET_DSA_TAG_BRCM
case DSA_TAG_PROTO_BRCM:
- ds->dst->ops = &brcm_netdev_ops;
+ p->xmit = brcm_netdev_ops.xmit;
break;
#endif
default:
- ds->dst->ops = ¬ag_netdev_ops;
+ p->xmit = dsa_slave_notag_xmit;
break;
}
- SET_NETDEV_DEV(slave_dev, parent);
- slave_dev->dev.of_node = ds->pd->port_dn[port];
- slave_dev->vlan_features = master->vlan_features;
-
- p = netdev_priv(slave_dev);
- p->dev = slave_dev;
- p->parent = ds;
- p->port = port;
-
p->old_pause = -1;
p->old_link = -1;
p->old_duplex = -1;
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 8fbc21c..83d3572 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -11,7 +11,6 @@
#include <linux/etherdevice.h>
#include <linux/list.h>
-#include <linux/netdevice.h>
#include <linux/slab.h>
#include "dsa_priv.h"
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index d7dbc5b..ce90c8b 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -10,7 +10,6 @@
#include <linux/etherdevice.h>
#include <linux/list.h>
-#include <linux/netdevice.h>
#include <linux/slab.h>
#include "dsa_priv.h"
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 6b30abe..94fcce7 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -10,7 +10,6 @@
#include <linux/etherdevice.h>
#include <linux/list.h>
-#include <linux/netdevice.h>
#include <linux/slab.h>
#include "dsa_priv.h"
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index 5fe9444..115fdca 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -10,7 +10,6 @@
#include <linux/etherdevice.h>
#include <linux/list.h>
-#include <linux/netdevice.h>
#include <linux/slab.h>
#include "dsa_priv.h"
^ permalink raw reply related
* Re: DSA and skb->protocol
From: Florian Fainelli @ 2014-09-15 17:08 UTC (permalink / raw)
To: Andrew Lunn; +Cc: seugene, Andrew Lunn, netdev, kernel
In-Reply-To: <20140914153751.GA28585@lunn.ch>
Hi Andrew,
On 09/14/2014 08:37 AM, Andrew Lunn wrote:
> Hi Florian
>
> I've been debugging a WARNING when using DSA with a D-Link
> DIR665. I've had reports of the same warning with another device.
>
> WARNING: CPU: 0 PID: 2014 at net/core/dev.c:2260 skb_warn_bad_offload+0xd0/0x104()
> mv643xx_eth_port: caps=(0x0000000400014803, 0x00000000001b482b) len=1722 data_len=16 gso_size=1448 gso_type=1 gso_segs 2 ip_summed=3 encapsulation 0 features 400014801
> Modules linked in:
> CPU: 0 PID: 2014 Comm: sshd Tainted: G W 3.17.0-rc1-00007-g2f06b2c08099-dirty #228
> [<c000e2a0>] (unwind_backtrace) from [<c000bd88>] (show_stack+0x10/0x14)
> [<c000bd88>] (show_stack) from [<c0016db8>] (warn_slowpath_common+0x6c/0x8c)
> [<c0016db8>] (warn_slowpath_common) from [<c0016e08>] (warn_slowpath_fmt+0x30/0x40)
> [<c0016e08>] (warn_slowpath_fmt) from [<c04ebad4>] (skb_warn_bad_offload+0xd0/0x104)
> [<c04ebad4>] (skb_warn_bad_offload) from [<c03eea5c>] (skb_checksum_help+0x160/0x170)
> [<c03eea5c>] (skb_checksum_help) from [<c03eef40>] (dev_hard_start_xmit+0x3f8/0x4c0)
> [<c03eef40>] (dev_hard_start_xmit) from [<c04071ec>] (sch_direct_xmit+0x148/0x250)
> [<c04071ec>] (sch_direct_xmit) from [<c03ef280>] (__dev_queue_xmit+0x278/0x5f4)
> [<c03ef280>] (__dev_queue_xmit) from [<c04719b8>] (edsa_xmit+0xf8/0x2c8)
> [<c04719b8>] (edsa_xmit) from [<c03eee24>] (dev_hard_start_xmit+0x2dc/0x4c0)
> [<c03eee24>] (dev_hard_start_xmit) from [<c03ef3cc>] (__dev_queue_xmit+0x3c4/0x5f4)
> [<c03ef3cc>] (__dev_queue_xmit) from [<c0415e88>] (ip_finish_output+0x64c/0x920)
> [<c0415e88>] (ip_finish_output) from [<c0416e2c>] (ip_local_out_sk+0x34/0x38)
> [<c0416e2c>] (ip_local_out_sk) from [<c0417144>] (ip_queue_xmit+0x128/0x388)
> [<c0417144>] (ip_queue_xmit) from [<c042caa8>] (tcp_transmit_skb+0x534/0x93c)
> [<c042caa8>] (tcp_transmit_skb) from [<c042cff8>] (tcp_write_xmit+0x148/0xbf8)
> [<c042cff8>] (tcp_write_xmit) from [<c042dd7c>] (__tcp_push_pending_frames+0x30/0x9c)
> [<c042dd7c>] (__tcp_push_pending_frames) from [<c041fbc8>] (tcp_sendmsg+0xc0/0xcec)
> [<c041fbc8>] (tcp_sendmsg) from [<c0445710>] (inet_sendmsg+0x3c/0x70)
> [<c0445710>] (inet_sendmsg) from [<c03d7d5c>] (sock_aio_write+0xcc/0xec)
> [<c03d7d5c>] (sock_aio_write) from [<c00bb218>] (do_sync_write+0x7c/0xa4)
> [<c00bb218>] (do_sync_write) from [<c00bbc40>] (vfs_write+0x108/0x1b0)
> [<c00bbc40>] (vfs_write) from [<c00bc214>] (SyS_write+0x40/0x94)
> [<c00bc214>] (SyS_write) from [<c0009480>] (ret_fast_syscall+0x0/0x2c)
> ---[ end trace b1b02d15aba4766a ]---
>
> I think i understand what is going on, and one of your recent patches
> may indicate you have seen something similar.
>
> int dev_hard_start_xmit() we have:
>
> 2607
> 2608 features = netif_skb_features(skb);
>
> ...
> 2637 /* If packet is not checksummed and device does not
> 2638 * support checksumming for this protocol, complete
> 2639 * checksumming here.
> 2640 */
> 2641 if (skb->ip_summed == CHECKSUM_PARTIAL) {
> 2642 if (skb->encapsulation)
> 2643 skb_set_inner_transport_header(skb,
> 2644 skb_checksum_start_offset(skb));
> 2645 else
> 2646 skb_set_transport_header(skb,
> 2647 skb_checksum_start_offset(skb));
> 2648 if (!(features & NETIF_F_ALL_CSUM) &&
> 2649 skb_checksum_help(skb))
> 2650 goto out_kfree_skb;
> 2651 }
>
> This packet is CHECKSUM_PARTIAL, but it is also a GSO packet, with two
> segments, so the skb_checksum_help() is throwing the warning. It is
> expected that the hardware with do the checksum when using GSO. The
> reason it is trying to do software checksums, not hardware, is because
> features does not indicate the protocol is supported by hardware
> checksumming. This i initially thought was odd, since this is a TCP/IP
> packet, as you can see from the stack trace. However,
>
> netif_skb_features(skb) calls harmonize_features() which calls
> can_checksum_protocol():
>
> 3206 static inline bool can_checksum_protocol(netdev_features_t features,
> 3207 __be16 protocol)
> 3208 {
> 3209 return ((features & NETIF_F_GEN_CSUM) ||
> 3210 ((features & NETIF_F_V4_CSUM) &&
> 3211 protocol == htons(ETH_P_IP)) ||
> 3212 ((features & NETIF_F_V6_CSUM) &&
> 3213 protocol == htons(ETH_P_IPV6)) ||
> 3214 ((features & NETIF_F_FCOE_CRC) &&
> 3215 protocol == htons(ETH_P_FCOE)));
> 3216 }
>
> However looking in the skb, we see protocol is now ETH_P_EDSA, not
> ETH_P_IP. Hence the hardware does not support this protocol.
Usually, the hardware needs to be told there is a DSA/EDSA tag before
the actual Ethernet frame, I don't have the mv643xx_eth documentation
handy, but I suppose there should be something like this available.
>
> This protocol value is because edsa_xmit() changes it in the slave
> device TX path.
>
> Your patch "net: dsa: change tag_protocol to an enum"
> has a hunk:
>
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index e0b759ec209e..8fbc21c0de78 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -91,7 +91,6 @@ static netdev_tx_t brcm_tag_xmit(struct sk_buff *skb, struct net_device *dev)
> /* Queue the SKB for transmission on the parent interface, but
> * do not modify its EtherType
> */
> - skb->protocol = htons(ETH_P_BRCMTAG);
> skb->dev = p->parent->dst->master_netdev;
> dev_queue_xmit(skb);
>
> making me think it is not actually needed to change
> skb->protocol. When i remove this from edsa_xmit(), the warning goes
> away and networking seems to work O.K.
>
> Is this the right fix? Should we remove this setting of skb->protocol
> in the other dsa xmit functions?
Adding Lennert here, I suspect that having the skb->protocol assignment
was initially done to help drivers such as mv643xx_eth and others doing
DSA to be able to identify these packets properly in the transmit path.
Unless there is something else, I would be inclined to remove the
skb->protocol assignment from tag_dsa.c and tag_edsa.c. In case the
driver needs to consult what is configured, it should either:
- look at dev->dsa_ptr->tag_protocol if we do not need to know on a
per-packet basis what's the tagging protocol used (using
netdev_uses_dsa() + a helper function we'd introduce)
- or, if we we need that information to be per-packet, have a
DSA-specific control block and a set of helpers to retrieve that information
--
Florian
^ permalink raw reply
* [PATCH 0/2] DSA Cleanups
From: Alexander Duyck @ 2014-09-15 16:59 UTC (permalink / raw)
To: netdev; +Cc: f.fainelli, kernel, davem
This patch series does two things, first it cleans up the tag_protocol and
protocol ops being configured seperately. Second it addresses the desire
to split DSA away from relying on a MII bus.
---
Alexander Duyck (2):
dsa: Split ops up, and avoid assigning tag_protocol and receive separately
dsa: Replace mii_bus with a generic host device
arch/arm/plat-orion/common.c | 2 +
drivers/net/dsa/bcm_sf2.c | 2 +
drivers/net/dsa/mv88e6060.c | 13 ++++++---
drivers/net/dsa/mv88e6123_61_65.c | 6 +++-
drivers/net/dsa/mv88e6131.c | 6 +++-
drivers/net/dsa/mv88e6171.c | 6 +++-
drivers/net/dsa/mv88e6xxx.c | 4 +--
include/linux/netdevice.h | 7 -----
include/net/dsa.h | 19 +++++++------
net/dsa/dsa.c | 56 ++++++++++++++++++++++++-------------
net/dsa/dsa_priv.h | 11 +++++++
net/dsa/slave.c | 39 +++++++++++---------------
net/dsa/tag_brcm.c | 1 -
net/dsa/tag_dsa.c | 1 -
net/dsa/tag_edsa.c | 1 -
net/dsa/tag_trailer.c | 1 -
16 files changed, 101 insertions(+), 74 deletions(-)
--
^ permalink raw reply
* Re: [PATCH] net: ethernet: marvell: sky2.c: Cleaning up missing null-terminate in conjunction with strncpy
From: David Miller @ 2014-09-15 17:07 UTC (permalink / raw)
To: stephen; +Cc: rickard_strandqvist, mlindner, netdev, linux-kernel
In-Reply-To: <20140914190557.690183d2@urahara>
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Sun, 14 Sep 2014 19:05:57 -0700
> On Sun, 14 Sep 2014 19:33:43 +0200
> Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> wrote:
>
>> Replacing strncpy with strlcpy to avoid strings that lacks null terminate.
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>> drivers/net/ethernet/marvell/sky2.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
>> index dba48a5c..7053d38 100644
>> --- a/drivers/net/ethernet/marvell/sky2.c
>> +++ b/drivers/net/ethernet/marvell/sky2.c
>> @@ -4907,7 +4907,7 @@ static const char *sky2_name(u8 chipid, char *buf, int sz)
>> };
>>
>> if (chipid >= CHIP_ID_YUKON_XL && chipid <= CHIP_ID_YUKON_OP_2)
>> - strncpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz);
>> + strlcpy(buf, name[chipid - CHIP_ID_YUKON_XL], sz);
>> else
>> snprintf(buf, sz, "(chip %#x)", chipid);
>> return buf;
>
> Useless and unnecessary since the list of names is right there.
> Why not avoid the copy all together?
>
> Subject: sky2: avoid strncpy
>
> Don't use strncpy() since security thought police think it is bad.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
I think providing the buffer on the stack of the thread executing the
probe is superior because it will allow enabling parallel probing
in the future.
I don't think you have to change that aspect to achieve your goal
of returning the const char * string when possible.
^ permalink raw reply
* Re: [PATCH v2 net-next 3/7] fou: Add GRO support
From: Or Gerlitz @ 2014-09-15 17:03 UTC (permalink / raw)
To: Tom Herbert; +Cc: David Miller, Linux Netdev List
In-Reply-To: <CA+mtBx-945Ku0OtcYGE195=R7cF5AHgacQeh8OPtC7NOQgMNvw@mail.gmail.com>
On Mon, Sep 15, 2014 at 6:10 PM, Tom Herbert <therbert@google.com> wrote:
> On Mon, Sep 15, 2014 at 8:00 AM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
>> On Mon, Sep 15, 2014 at 6:07 AM, Tom Herbert <therbert@google.com> wrote:
>>> Implement fou_gro_receive and fou_gro_complete, and populate these
>>> in the correponsing udp_offloads for the socket. Added ipproto to
>>> udp_offloads and pass this from UDP to the fou GRO routine in proto
>>> field of napi_gro_cb structure.
>>
>>
>> Do we really need that extra hop of fou4_gro_receive/complete?
>> can't we somehow plant the gro receive/complete (say) GRE handlers in
>> the udp offload
>> struct with the UDP port that related to (say) GRE over UDP tunneling?
>>
> That would be nice, but it isn't obvious to me how to manage the
> references. The offload functions are accessed with RCU pretty consistently.
Currently udp_gro_receive calls rcu_read_lock() before it invokes
fou4/6_gro_receive
and rcu_read_unlock after the fou calls returns. The Fou call repeats
the same practice
w.r.t the (say) GRE gro receive callback, so... what happens if we
eliminate the fou part all together?
^ permalink raw reply
* Re: [PATCH 2/4 linux-next] bna: use container_of to resolve bufdesc_ex from bufdesc
From: David Miller @ 2014-09-15 16:49 UTC (permalink / raw)
To: fabf; +Cc: linux-kernel, rmody, netdev
In-Reply-To: <1410640709-18295-3-git-send-email-fabf@skynet.be>
From: Fabian Frederick <fabf@skynet.be>
Date: Sat, 13 Sep 2014 22:38:27 +0200
> Use container_of instead of casting first structure member.
>
> Compiled but untested.
>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
Applied.
^ permalink raw reply
* Re: [PATCH 1/4 linux-next] net: fec: use container_of to resolve bufdesc_ex from bufdesc
From: David Miller @ 2014-09-15 16:49 UTC (permalink / raw)
To: fabf; +Cc: linux-kernel, netdev
In-Reply-To: <1410640709-18295-2-git-send-email-fabf@skynet.be>
From: Fabian Frederick <fabf@skynet.be>
Date: Sat, 13 Sep 2014 22:38:26 +0200
> Use container_of instead of casting first structure member.
>
> ARM cross-compiled but untested.
>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
Applied.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox