* Re: [patch net-next v3 01/17] bridge: rename fdb_*_hw to fdb_*_hw_addr to avoid confusion
From: Florian Fainelli @ 2014-11-25 22:33 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, roopa, linville,
jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh,
aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye, simon.horman,
alexander.h.duyck, john.ronciak, mleitner, shrijeet, gospo, bcrl
In-Reply-To: <1416911328-10979-2-git-send-email-jiri@resnulli.us>
On 25/11/14 02:28, Jiri Pirko wrote:
> The current name might seem that this actually offloads the fdb entry to
> hw. So rename it to clearly present that this for hardware address
> addition/removal.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
FWIW:
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
^ permalink raw reply
* Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device
From: Thomas Graf @ 2014-11-25 22:36 UTC (permalink / raw)
To: Andy Gospodarek
Cc: Jiri Pirko, netdev, davem, nhorman, andy, dborkman, ogerlitz,
jesse, pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, jhs, sfeldma,
f.fainelli, roopa, linville, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, bcrl
In-Reply-To: <20141125163853.GJ27416@gospo.rtplab.test>
On 11/25/14 at 11:38am, Andy Gospodarek wrote:
> On Tue, Nov 25, 2014 at 11:28:40AM +0100, Jiri Pirko wrote:
> > From: Scott Feldman <sfeldma@gmail.com>
> >
> > When the swdev device learns a new mac/vlan on a port, it sends some async
> > notification to the driver and the driver installs an FDB in the device.
> > To give a holistic system view, the learned mac/vlan should be reflected
> > in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
> > what is currently learned by the device. This API on the bridge driver gives
> > a way for the swdev driver to install an FBD entry in the bridge FBD table.
> > (And remove one).
> >
> > This is equivalent to the device running these cmds:
> >
> > bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
> >
> > This patch needs some extra eyeballs for review, in paricular around the
> > locking and contexts.
> >
> > Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> > Signed-off-by: Jiri Pirko <jiri@resnulli.us>
I like the simplicity of this. That said, given we'll have multiple
users of swdev including OVS, shouldn't this be a notifier or a
callback that depends on who is controlling the device?
> > + spin_lock(&br->hash_lock);
> (Since you asked to check locking...)
>
> Most of the other fdb_add/delete/insert/update calls take this with
> spin_lock_bh. Did you try this with lockdep enabled just to see if that
> is needed here? I suspect that anytime br->hash_lock is taken it will
> need to be with softirqs disabled from this point forward.
At least br_fdb_update() seems to be called from BH context so I would
agree and argue the lock in br_fdb_cleanup() and br_fdb_update() need a
fix too. I'll send a patch.
^ permalink raw reply
* Re: [patch net-next v3 02/17] net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del
From: Florian Fainelli @ 2014-11-25 22:39 UTC (permalink / raw)
To: Thomas Graf, Jiri Pirko
Cc: netdev, davem, nhorman, andy, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, roopa, linville,
jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh,
aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye, simon.horman,
alexander.h.duyck, john.ronciak, mleitner, shrijeet, gospo, bcrl
In-Reply-To: <20141125221417.GF3912@casper.infradead.org>
On 25/11/14 14:14, Thomas Graf wrote:
> On 11/25/14 at 11:28am, Jiri Pirko wrote:
>> Do the work of parsing NDA_VLAN directly in rtnetlink code, pass simple
>> u16 vid to drivers from there.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
> I'm slightly confused ;-)
>
> We both argued that parsing Netlink attributes in the drivers is wrong.
> What happened to the plan of renaming ndo_fdb_ to ndo_neigh_ and
> introducing a non-Netlink in-kernel API for advanced usage by swdev?
>
Not sure I follow you here, the commit message says what it says it
does, are we looking at the same patch?
--
Florian
^ permalink raw reply
* [net-next PATCH 0/5] net: Add helper for padding short Ethernet frames
From: Alexander Duyck @ 2014-11-25 22:43 UTC (permalink / raw)
To: netdev; +Cc: davem
This patch series adds a helper to pad short Ethernet frames. The general
idea is to clean up a number of code paths that were all writing their own
versions of the same or similar function.
An added advantage is that this will help to discourage introducing new
bugs as in at least one case I found the skb->len had been updated, but the
tail pointer update was overlooked.
---
Alexander Duyck (5):
etherdevice: Add function for handling padding frame to ETH_ZLEN
ethernet/intel: Use eth_skb_pad helper
niu: Use eth_skb_pad helper
myri10ge: use eth_skb_pad helper
r8169: Use eth_skb_pad function
drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------
drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++--------
drivers/net/ethernet/intel/igb/igb_main.c | 11 +++--------
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++--------
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++--------
drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 15 +++++----------
drivers/net/ethernet/realtek/r8169.c | 12 ++----------
drivers/net/ethernet/sun/niu.c | 9 ++-------
include/linux/etherdevice.h | 21 +++++++++++++++++++++
9 files changed, 44 insertions(+), 65 deletions(-)
--
^ permalink raw reply
* [net-next PATCH 1/5] etherdevice: Add function for handling padding frame to ETH_ZLEN
From: Alexander Duyck @ 2014-11-25 22:44 UTC (permalink / raw)
To: netdev; +Cc: davem
In-Reply-To: <20141125223727.1867.43890.stgit@ahduyck-vm-fedora20>
This patch adds a simple function for padding a frame up to the minimum
size for for Ethernet. The motivation behind it is that there are a number
of implementations throughout the network device drivers that are all doing
the same thing, but each a little bit differently and as a result several
implementations contain bugs such as updating the length without updating
the tail offset and other similar issues.
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
include/linux/etherdevice.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 733980f..7e436f3 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -392,4 +392,25 @@ static inline unsigned long compare_ether_header(const void *a, const void *b)
#endif
}
+/**
+ * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
+ * @skb: Buffer to pad
+ *
+ * An Ethernet frame should have a minimum size of 60 bytes. This function
+ * takes short frames and pads them with zeros up to the 60 byte limit.
+ */
+static inline int eth_skb_pad(struct sk_buff *skb)
+{
+ unsigned int size = skb->len;
+
+ if (unlikely(size < ETH_ZLEN)) {
+ size = ETH_ZLEN - size;
+ if (skb_pad(skb, size))
+ return -ENOMEM;
+ __skb_put(skb, size);
+ }
+
+ return 0;
+}
+
#endif /* _LINUX_ETHERDEVICE_H */
^ permalink raw reply related
* [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper
From: Alexander Duyck @ 2014-11-25 22:44 UTC (permalink / raw)
To: netdev; +Cc: Jeff Kirsher, davem
In-Reply-To: <20141125223727.1867.43890.stgit@ahduyck-vm-fedora20>
Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
their own implementations of the function.
Also this cleans up two other spots where skb_pad was called but the length
and tail pointers were being manipulated directly instead of just having
the padding length added via __skb_put.
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------
drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++--------
drivers/net/ethernet/intel/igb/igb_main.c | 11 +++--------
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++--------
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++--------
5 files changed, 14 insertions(+), 38 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 24f3986..862d198 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
* packets may get corrupted during padding by HW.
* To WA this issue, pad all small packets manually.
*/
- if (skb->len < ETH_ZLEN) {
- if (skb_pad(skb, ETH_ZLEN - skb->len))
- return NETDEV_TX_OK;
- skb->len = ETH_ZLEN;
- skb_set_tail_pointer(skb, ETH_ZLEN);
- }
+ if (eth_skb_pad(skb))
+ return NETDEV_TX_OK;
mss = skb_shinfo(skb)->gso_size;
/* The controller does a simple calculation to
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 73457ed..91516ae 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -578,14 +578,9 @@ static bool fm10k_cleanup_headers(struct fm10k_ring *rx_ring,
if (skb_is_nonlinear(skb))
fm10k_pull_tail(rx_ring, rx_desc, skb);
- /* if skb_pad returns an error the skb was freed */
- if (unlikely(skb->len < 60)) {
- int pad_len = 60 - skb->len;
-
- if (skb_pad(skb, pad_len))
- return true;
- __skb_put(skb, pad_len);
- }
+ /* if eth_skb_pad returns an error the skb was freed */
+ if (eth_skb_pad(skb))
+ return true;
return false;
}
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b0e12e7..3cb54d0 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6843,14 +6843,9 @@ static bool igb_cleanup_headers(struct igb_ring *rx_ring,
if (skb_is_nonlinear(skb))
igb_pull_tail(rx_ring, rx_desc, skb);
- /* if skb_pad returns an error the skb was freed */
- if (unlikely(skb->len < 60)) {
- int pad_len = 60 - skb->len;
-
- if (skb_pad(skb, pad_len))
- return true;
- __skb_put(skb, pad_len);
- }
+ /* if eth_skb_pad returns an error the skb was freed */
+ if (eth_skb_pad(skb))
+ return true;
return false;
}
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 932f779..7d0991a 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1766,14 +1766,9 @@ static bool ixgbe_cleanup_headers(struct ixgbe_ring *rx_ring,
return false;
#endif
- /* if skb_pad returns an error the skb was freed */
- if (unlikely(skb->len < 60)) {
- int pad_len = 60 - skb->len;
-
- if (skb_pad(skb, pad_len))
- return true;
- __skb_put(skb, pad_len);
- }
+ /* if eth_skb_pad returns an error the skb was freed */
+ if (eth_skb_pad(skb))
+ return true;
return false;
}
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 755f71f..465d6a8 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -612,14 +612,9 @@ static bool ixgbevf_cleanup_headers(struct ixgbevf_ring *rx_ring,
if (skb_is_nonlinear(skb))
ixgbevf_pull_tail(rx_ring, skb);
- /* if skb_pad returns an error the skb was freed */
- if (unlikely(skb->len < 60)) {
- int pad_len = 60 - skb->len;
-
- if (skb_pad(skb, pad_len))
- return true;
- __skb_put(skb, pad_len);
- }
+ /* if eth_skb_pad returns an error the skb was freed */
+ if (eth_skb_pad(skb))
+ return true;
return false;
}
^ permalink raw reply related
* [net-next PATCH 3/5] niu: Use eth_skb_pad helper
From: Alexander Duyck @ 2014-11-25 22:44 UTC (permalink / raw)
To: netdev; +Cc: davem
In-Reply-To: <20141125223727.1867.43890.stgit@ahduyck-vm-fedora20>
Replace the standard layout for padding an ethernet frame with the
eth_skb_pad call.
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
drivers/net/ethernet/sun/niu.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 904fd1a..4aaa324 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -6651,13 +6651,8 @@ static netdev_tx_t niu_start_xmit(struct sk_buff *skb,
return NETDEV_TX_BUSY;
}
- if (skb->len < ETH_ZLEN) {
- unsigned int pad_bytes = ETH_ZLEN - skb->len;
-
- if (skb_pad(skb, pad_bytes))
- goto out;
- skb_put(skb, pad_bytes);
- }
+ if (eth_skb_pad(skb))
+ goto out;
len = sizeof(struct tx_pkt_hdr) + 15;
if (skb_headroom(skb) < len) {
^ permalink raw reply related
* [net-next PATCH 4/5] myri10ge: use eth_skb_pad helper
From: Alexander Duyck @ 2014-11-25 22:44 UTC (permalink / raw)
To: netdev; +Cc: Hyong-Youb Kim, davem
In-Reply-To: <20141125223727.1867.43890.stgit@ahduyck-vm-fedora20>
Update myri10ge to use eth_skb_pad helper. This also corrects a minor
issue as the driver was updating length without updating the tail pointer.
Cc: Hyong-Youb Kim <hykim@myri.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
index 9e7e3f1..af09905 100644
--- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
+++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
@@ -2913,16 +2913,11 @@ again:
flags |= MXGEFW_FLAGS_SMALL;
/* pad frames to at least ETH_ZLEN bytes */
- if (unlikely(skb->len < ETH_ZLEN)) {
- if (skb_padto(skb, ETH_ZLEN)) {
- /* The packet is gone, so we must
- * return 0 */
- ss->stats.tx_dropped += 1;
- return NETDEV_TX_OK;
- }
- /* adjust the len to account for the zero pad
- * so that the nic can know how long it is */
- skb->len = ETH_ZLEN;
+ if (eth_skb_pad(skb)) {
+ /* The packet is gone, so we must
+ * return 0 */
+ ss->stats.tx_dropped += 1;
+ return NETDEV_TX_OK;
}
}
^ permalink raw reply related
* [net-next PATCH 5/5] r8169: Use eth_skb_pad function
From: Alexander Duyck @ 2014-11-25 22:44 UTC (permalink / raw)
To: netdev; +Cc: Realtek linux nic maintainers, davem
In-Reply-To: <20141125223727.1867.43890.stgit@ahduyck-vm-fedora20>
Replace rtl_skb_pad with eth_skb_pad since they do the same thing.
Cc: Realtek linux nic maintainers <nic_swsd@realtek.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
drivers/net/ethernet/realtek/r8169.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index cf154f7..7ff4646 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -6832,14 +6832,6 @@ err_out:
return -EIO;
}
-static bool rtl_skb_pad(struct sk_buff *skb)
-{
- if (skb_padto(skb, ETH_ZLEN))
- return false;
- skb_put(skb, ETH_ZLEN - skb->len);
- return true;
-}
-
static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp, struct sk_buff *skb)
{
return skb->len < ETH_ZLEN && tp->mac_version == RTL_GIGA_MAC_VER_34;
@@ -6980,7 +6972,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
u8 ip_protocol;
if (unlikely(rtl_test_hw_pad_bug(tp, skb)))
- return skb_checksum_help(skb) == 0 && rtl_skb_pad(skb);
+ return skb_checksum_help(skb) == 0 && eth_skb_pad(skb);
if (transport_offset > TCPHO_MAX) {
netif_warn(tp, tx_err, tp->dev,
@@ -7015,7 +7007,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
opts[1] |= transport_offset << TCPHO_SHIFT;
} else {
if (unlikely(rtl_test_hw_pad_bug(tp, skb)))
- return rtl_skb_pad(skb);
+ return eth_skb_pad(skb);
}
return true;
^ permalink raw reply related
* Re: [patch net-next v3 09/17] bridge: add API to notify bridge driver of learned FBD on offloaded device
From: Florian Fainelli @ 2014-11-25 22:44 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, roopa, linville,
jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh,
aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye, simon.horman,
alexander.h.duyck, john.ronciak, mleitner, shrijeet, gospo, bcrl
In-Reply-To: <1416911328-10979-10-git-send-email-jiri@resnulli.us>
On 25/11/14 02:28, Jiri Pirko wrote:
> From: Scott Feldman <sfeldma@gmail.com>
>
> When the swdev device learns a new mac/vlan on a port, it sends some async
> notification to the driver and the driver installs an FDB in the device.
> To give a holistic system view, the learned mac/vlan should be reflected
> in the bridge's FBD table, so the user, using normal iproute2 cmds, can view
> what is currently learned by the device. This API on the bridge driver gives
> a way for the swdev driver to install an FBD entry in the bridge FBD table.
> (And remove one).
>
> This is equivalent to the device running these cmds:
>
> bridge fdb [add|del] <mac> dev <dev> vid <vlan id> master
>
> This patch needs some extra eyeballs for review, in paricular around the
> locking and contexts.
>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
[snip]
> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index 4a1d7e9..3a9b0df 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -40,6 +40,7 @@ enum {
>
> #define NTF_SELF 0x02
> #define NTF_MASTER 0x04
> +#define NTF_EXT_LEARNED 0x10
This file could use some re-ordering to have the constants in ascending
order.
[snip]
> + head = &br->hash[br_mac_hash(addr, vid)];
> + fdb = fdb_find(head, addr, vid);
> + if (!fdb) {
> + fdb = fdb_create(head, p, addr, vid);
> + if (!fdb) {
> + err = -ENOMEM;
> + goto err_unlock;
> + }
> + fdb->added_by_external_learn = 1;
> + fdb_notify(br, fdb, RTM_NEWNEIGH);
> + } else if (fdb->added_by_external_learn) {
> + /* Refresh entry */
> + fdb->updated = fdb->used = jiffies;
> + } else if (!fdb->added_by_user) {
> + /* Take over SW learned entry */
> + fdb->added_by_external_learn = 1;
> + fdb->updated = jiffies;
> + fdb_notify(br, fdb, RTM_NEWNEIGH);
> + }
Is there any case where this fdb entry gets re-used and is no longer
added by an external learning? Should we clear this flag somewhere?
[snip]
> +EXPORT_SYMBOL(br_fdb_external_learn_del);
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 4f577c4..02cd63b 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -101,6 +101,7 @@ struct net_bridge_fdb_entry
> unsigned char is_local;
> unsigned char is_static;
> unsigned char added_by_user;
> + unsigned char added_by_external_learn;
Pheww, we could be saving some memory footprint here by using different
types here ;)
--
Florian
^ permalink raw reply
* Re: [patch net-next v3 08/17] bridge: call netdev_sw_port_stp_update when bridge port STP status changes
From: Roopa Prabhu @ 2014-11-25 22:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, jhs, sfeldma,
f.fainelli, linville, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, gospo, bcrl
In-Reply-To: <1416911328-10979-9-git-send-email-jiri@resnulli.us>
On 11/25/14, 2:28 AM, Jiri Pirko wrote:
> From: Scott Feldman <sfeldma@gmail.com>
>
> To notify switch driver of change in STP state of bridge port, add new
> .ndo op and provide switchdev wrapper func to call ndo op. Use it in bridge
> code then.
>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> v2->v3:
> -changed "sw" string to "switch" to avoid confusion
> v1->v2:
> -no change
> ---
> include/linux/netdevice.h | 5 +++++
> include/net/switchdev.h | 7 +++++++
> net/bridge/br_stp.c | 2 ++
> net/switchdev/switchdev.c | 19 +++++++++++++++++++
> 4 files changed, 33 insertions(+)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index ce096dc..66cb64e 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1024,6 +1024,9 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> * Called to get an ID of the switch chip this port is part of.
> * If driver implements this, it indicates that it represents a port
> * of a switch chip.
> + * int (*ndo_switch_port_stp_update)(struct net_device *dev, u8 state);
> + * Called to notify switch device port of bridge port STP
> + * state change.
> */
> struct net_device_ops {
> int (*ndo_init)(struct net_device *dev);
> @@ -1180,6 +1183,8 @@ struct net_device_ops {
> #ifdef CONFIG_NET_SWITCHDEV
> int (*ndo_switch_parent_id_get)(struct net_device *dev,
> struct netdev_phys_item_id *psid);
> + int (*ndo_switch_port_stp_update)(struct net_device *dev,
> + u8 state);
> #endif
> };
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 7a52360..8a6d164 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -16,6 +16,7 @@
>
> int netdev_switch_parent_id_get(struct net_device *dev,
> struct netdev_phys_item_id *psid);
> +int netdev_switch_port_stp_update(struct net_device *dev, u8 state);
>
> #else
>
> @@ -25,6 +26,12 @@ static inline int netdev_switch_parent_id_get(struct net_device *dev,
> return -EOPNOTSUPP;
> }
>
> +static inline int netdev_switch_port_stp_update(struct net_device *dev,
> + u8 state)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> #endif
>
> #endif /* _LINUX_SWITCHDEV_H_ */
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index 2b047bc..35e016c 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -12,6 +12,7 @@
> */
> #include <linux/kernel.h>
> #include <linux/rculist.h>
> +#include <net/switchdev.h>
>
> #include "br_private.h"
> #include "br_private_stp.h"
> @@ -39,6 +40,7 @@ void br_log_state(const struct net_bridge_port *p)
> void br_set_state(struct net_bridge_port *p, unsigned int state)
> {
> p->state = state;
> + netdev_switch_port_stp_update(p->dev, state);
> }
>
> /* called under bridge lock */
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 66973de..d162b21 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -31,3 +31,22 @@ int netdev_switch_parent_id_get(struct net_device *dev,
> return ops->ndo_switch_parent_id_get(dev, psid);
> }
> EXPORT_SYMBOL(netdev_switch_parent_id_get);
> +
> +/**
> + * netdev_switch_port_stp_update - Notify switch device port of STP
> + * state change
> + * @dev: port device
> + * @state: port STP state
> + *
> + * Notify switch device port of bridge port STP state change.
> + */
> +int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
> +{
> + const struct net_device_ops *ops = dev->netdev_ops;
> +
> + if (!ops->ndo_switch_port_stp_update)
> + return -EOPNOTSUPP;
> + WARN_ON(!ops->ndo_switch_parent_id_get);
> + return ops->ndo_switch_port_stp_update(dev, state);
> +}
> +EXPORT_SYMBOL(netdev_switch_port_stp_update);
This should also check if offload is enabled on the bridge/port ?
^ permalink raw reply
* Re: [patch net-next v3 13/17] bridge: move private brport flags to if_bridge.h so port drivers can use flags
From: Thomas Graf @ 2014-11-25 22:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, nhorman, andy, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, f.fainelli, roopa,
linville, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck, john.ronciak, mleitner, shrijeet,
gospo, bcrl
In-Reply-To: <1416911328-10979-14-git-send-email-jiri@resnulli.us>
On 11/25/14 at 11:28am, Jiri Pirko wrote:
> From: Scott Feldman <sfeldma@gmail.com>
>
> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Could be ported to BIT() if you respin.
^ permalink raw reply
* Re: [patch net-next v3 15/17] bridge: add new hwmode swdev
From: Thomas Graf @ 2014-11-25 22:56 UTC (permalink / raw)
To: Andy Gospodarek
Cc: Roopa Prabhu, Jiri Pirko, netdev, davem, nhorman, andy, dborkman,
ogerlitz, jesse, pshelar, azhou, ben, stephen, jeffrey.t.kirsher,
vyasevic, xiyou.wangcong, john.r.fastabend, edumazet, jhs,
sfeldma, f.fainelli, linville, jasowang, ebiederm,
nicolas.dichtel, ryazanov.s.a, buytenh, aviadr, nbd,
alexei.starovoitov, Neil.Jerram, ronye, simon.horman,
alexander.h.duyck, john.ronciak, mleitner, shrijeet, bcrl@
In-Reply-To: <20141125161733.GI27416@gospo.rtplab.test>
On 11/25/14 at 11:17am, Andy Gospodarek wrote:
> On Tue, Nov 25, 2014 at 08:03:11AM -0800, Roopa Prabhu wrote:
> > On 11/25/14, 2:28 AM, Jiri Pirko wrote:
> > >From: Scott Feldman <sfeldma@gmail.com>
> > >
> > >Current hwmode settings are "vepa" or "veb". These are for NIC interfaces
> > >with basic bridging function offloaded to HW. Add new "swdev" for full
> > >switch device offloads.
> >
> > still voting for a generic feature flag like NETIF_F_HW_OFFLOAD for use by
> > bridge/bond/vxlan. I can resubmit my patch that does this.
>
> I know it is _early_ to propose this, but I would even suggest this:
>
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index 8e30685..6cea162 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -124,6 +124,7 @@ enum {
> #define NETIF_F_HW_VLAN_STAG_TX __NETIF_F(HW_VLAN_STAG_TX)
> #define NETIF_F_HW_L2FW_DOFFLOAD __NETIF_F(HW_L2FW_DOFFLOAD)
> #define NETIF_F_BUSY_POLL __NETIF_F(BUSY_POLL)
> +#define NETIF_F_HW_SWITCHING __NETIF_F(HW_SWITCHING)
>
> /* Features valid for ethtool to change */
> /* = all defined minus driver/device-class-related */
I need more context to understand the intent of this. What does the
presence of the flag indicate? Support for swdev API? My
understanding is that the API will grow quickly and device will
only support a subset of what the API can abstract.
^ permalink raw reply
* Re: [patch net-next v3 15/17] bridge: add new hwmode swdev
From: Thomas Graf @ 2014-11-25 23:00 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Jiri Pirko, netdev, davem, nhorman, andy, dborkman, ogerlitz,
jesse, pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, sfeldma, f.fainelli,
roopa, linville, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, gospo, bcrl
In-Reply-To: <5474A95B.3080607@mojatatu.com>
On 11/25/14 at 11:07am, Jamal Hadi Salim wrote:
> On 11/25/14 05:28, Jiri Pirko wrote:
> >From: Scott Feldman <sfeldma@gmail.com>
> >
> >Current hwmode settings are "vepa" or "veb". These are for NIC interfaces
> >with basic bridging function offloaded to HW. Add new "swdev" for full
> >switch device offloads.
> >
> >Signed-off-by: Scott Feldman <sfeldma@gmail.com>
> >Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >---
> >new in v3
> >---
> > include/uapi/linux/if_bridge.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> >diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> >index da17e45..60425ca 100644
> >--- a/include/uapi/linux/if_bridge.h
> >+++ b/include/uapi/linux/if_bridge.h
> >@@ -105,6 +105,7 @@ struct __fdb_entry {
> >
> > #define BRIDGE_MODE_VEB 0 /* Default loopback mode */
> > #define BRIDGE_MODE_VEPA 1 /* 802.1Qbg defined VEPA mode */
> >+#define BRIDGE_MODE_SWDEV 2 /* Full switch device offload */
> >
> > /* Bridge management nested attributes
> > * [IFLA_AF_SPEC] = {
> >
>
> Again - Why is this not a generic interface the way Roopa had it?
> We need to do offloads for a lot of other things than just bridge..
I agree in general but I'm not sure we should mirror the NETIF_F_*
approach. Even the relatively simple encap offload has turned into
a complex set of feature bitmasks requiring harmonization.
I think we should discuss this in the context of John's work which
neatly abstracts the capabilities.
^ permalink raw reply
* Re: [patch net-next v3 02/17] net: make vid as a parameter for ndo_fdb_add/ndo_fdb_del
From: Thomas Graf @ 2014-11-25 23:11 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jiri Pirko, netdev, davem, nhorman, andy, dborkman, ogerlitz,
jesse, pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, jhs, sfeldma, roopa,
linville, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck, john.ronciak, mleitner, shrijeet,
gospo, bcrl
In-Reply-To: <54750505.3060004@gmail.com>
On 11/25/14 at 02:39pm, Florian Fainelli wrote:
> On 25/11/14 14:14, Thomas Graf wrote:
> > On 11/25/14 at 11:28am, Jiri Pirko wrote:
> >> Do the work of parsing NDA_VLAN directly in rtnetlink code, pass simple
> >> u16 vid to drivers from there.
> >>
> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >
> > I'm slightly confused ;-)
> >
> > We both argued that parsing Netlink attributes in the drivers is wrong.
> > What happened to the plan of renaming ndo_fdb_ to ndo_neigh_ and
> > introducing a non-Netlink in-kernel API for advanced usage by swdev?
> >
>
> Not sure I follow you here, the commit message says what it says it
> does, are we looking at the same patch?
I'm referring to the discussion that occured on patch 06/10 of v2:
http://www.spinics.net/lists/netdev/msg303637.html
http://www.spinics.net/lists/netdev/msg303669.html
http://www.spinics.net/lists/netdev/msg303689.html
http://www.spinics.net/lists/netdev/msg303706.html
I won't hold up this series though. Glad to do the work afterwards.
^ permalink raw reply
* Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper
From: Eric Dumazet @ 2014-11-25 23:14 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, Jeff Kirsher, davem
In-Reply-To: <20141125224406.1867.97911.stgit@ahduyck-vm-fedora20>
On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote:
> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
> their own implementations of the function.
>
> Also this cleans up two other spots where skb_pad was called but the length
> and tail pointers were being manipulated directly instead of just having
> the padding length added via __skb_put.
>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
> drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------
> drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++--------
> drivers/net/ethernet/intel/igb/igb_main.c | 11 +++--------
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++--------
> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++--------
> 5 files changed, 14 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index 24f3986..862d198 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
> * packets may get corrupted during padding by HW.
> * To WA this issue, pad all small packets manually.
> */
> - if (skb->len < ETH_ZLEN) {
> - if (skb_pad(skb, ETH_ZLEN - skb->len))
> - return NETDEV_TX_OK;
> - skb->len = ETH_ZLEN;
> - skb_set_tail_pointer(skb, ETH_ZLEN);
> - }
> + if (eth_skb_pad(skb))
> + return NETDEV_TX_OK;
>
Its a bit sad almost no driver increments some drop counter.
This probably could be generically done in eth_skb_pad()
atomic_long_inc(&skb->dev->tx_dropped)
^ permalink raw reply
* Re: [PATCH v5 2/4] arch: Add lightweight memory barriers dma_rmb() and dma_wmb()
From: Benjamin Herrenschmidt @ 2014-11-25 23:15 UTC (permalink / raw)
To: Alexander Duyck
Cc: linux-arch, netdev, linux-kernel, mathieu.desnoyers, peterz,
heiko.carstens, mingo, mikey, linux, donald.c.skidmore,
matthew.vick, geert, jeffrey.t.kirsher, romieu, paulmck, nic_swsd,
will.deacon, michael, tony.luck, torvalds, oleg, schwidefsky,
fweisbec, davem
In-Reply-To: <20141119012400.9563.21117.stgit@ahduyck-server>
On Tue, 2014-11-18 at 17:24 -0800, Alexander Duyck wrote:
> There are a number of situations where the mandatory barriers rmb() and
> wmb() are used to order memory/memory operations in the device drivers
> and those barriers are much heavier than they actually need to be. For
> example in the case of PowerPC wmb() calls the heavy-weight sync
> instruction when for coherent memory operations all that is really needed
> is an lsync or eieio instruction.
> .../...
For powerpc:
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
^ permalink raw reply
* Re: [net-next PATCH 5/5] r8169: Use eth_skb_pad function
From: Francois Romieu @ 2014-11-26 0:02 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, Realtek linux nic maintainers, davem
In-Reply-To: <20141125224425.1867.70460.stgit@ahduyck-vm-fedora20>
Alexander Duyck <alexander.h.duyck@redhat.com> :
> Replace rtl_skb_pad with eth_skb_pad since they do the same thing.
rtl_skb_pad returns true on success.
eth_skb_pad returns 0 on success.
--
Ueimor
^ permalink raw reply
* Re: [net-next PATCH 5/5] r8169: Use eth_skb_pad function
From: Alexander Duyck @ 2014-11-26 0:33 UTC (permalink / raw)
To: Francois Romieu, Alexander Duyck
Cc: netdev, Realtek linux nic maintainers, davem
In-Reply-To: <20141126000258.GA5901@electric-eye.fr.zoreil.com>
On 11/25/2014 04:02 PM, Francois Romieu wrote:
> Alexander Duyck <alexander.h.duyck@redhat.com> :
>> Replace rtl_skb_pad with eth_skb_pad since they do the same thing.
> rtl_skb_pad returns true on success.
>
> eth_skb_pad returns 0 on success.
>
Okay, that is an easy enough fix. I will just test for !eth_skb_pad in
the v2 version.
- Alex
^ permalink raw reply
* Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper
From: Alexander Duyck @ 2014-11-26 0:44 UTC (permalink / raw)
To: Eric Dumazet, Alexander Duyck; +Cc: netdev, Jeff Kirsher, davem
In-Reply-To: <1416957251.29427.39.camel@edumazet-glaptop2.roam.corp.google.com>
On 11/25/2014 03:14 PM, Eric Dumazet wrote:
> On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote:
>> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
>> their own implementations of the function.
>>
>> Also this cleans up two other spots where skb_pad was called but the length
>> and tail pointers were being manipulated directly instead of just having
>> the padding length added via __skb_put.
>>
>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>> ---
>> drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------
>> drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++--------
>> drivers/net/ethernet/intel/igb/igb_main.c | 11 +++--------
>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++--------
>> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++--------
>> 5 files changed, 14 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
>> index 24f3986..862d198 100644
>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
>> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
>> * packets may get corrupted during padding by HW.
>> * To WA this issue, pad all small packets manually.
>> */
>> - if (skb->len < ETH_ZLEN) {
>> - if (skb_pad(skb, ETH_ZLEN - skb->len))
>> - return NETDEV_TX_OK;
>> - skb->len = ETH_ZLEN;
>> - skb_set_tail_pointer(skb, ETH_ZLEN);
>> - }
>> + if (eth_skb_pad(skb))
>> + return NETDEV_TX_OK;
>>
> Its a bit sad almost no driver increments some drop counter.
>
> This probably could be generically done in eth_skb_pad()
>
> atomic_long_inc(&skb->dev->tx_dropped)
The only problem is eth_skb_pad is called in the Rx path of some drivers
as well.
I wonder if we couldn't make this some sort of netdevice attribute to
indicate what the smallest frame we can handle is and then just pad the
frame to that as a part of __dev_xmit_skb. Then we could do that
outside of the locks and take care of it before we even hit the qdisc layer.
- Alex
^ permalink raw reply
* Re: [net-next PATCH 1/5] etherdevice: Add function for handling padding frame to ETH_ZLEN
From: Florian Fainelli @ 2014-11-26 0:56 UTC (permalink / raw)
To: Alexander Duyck, netdev; +Cc: davem
In-Reply-To: <20141125224400.1867.48907.stgit@ahduyck-vm-fedora20>
On 25/11/14 14:44, Alexander Duyck wrote:
> This patch adds a simple function for padding a frame up to the minimum
> size for for Ethernet. The motivation behind it is that there are a number
> of implementations throughout the network device drivers that are all doing
> the same thing, but each a little bit differently and as a result several
> implementations contain bugs such as updating the length without updating
> the tail offset and other similar issues.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
> ---
> include/linux/etherdevice.h | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index 733980f..7e436f3 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -392,4 +392,25 @@ static inline unsigned long compare_ether_header(const void *a, const void *b)
> #endif
> }
>
> +/**
> + * eth_skb_pad - Pad buffer to mininum number of octets for Ethernet frame
> + * @skb: Buffer to pad
> + *
> + * An Ethernet frame should have a minimum size of 60 bytes. This function
> + * takes short frames and pads them with zeros up to the 60 byte limit.
minimum size without FCS
> + */
> +static inline int eth_skb_pad(struct sk_buff *skb)
> +{
> + unsigned int size = skb->len;
> +
> + if (unlikely(size < ETH_ZLEN)) {
> + size = ETH_ZLEN - size;
> + if (skb_pad(skb, size))
> + return -ENOMEM;
> + __skb_put(skb, size);
> + }
most drivers call skb_padto(skb, ETH_ZLEN), besides the extra
__skb_put() here, this is not different.
> +
> + return 0;
> +}
> +
> #endif /* _LINUX_ETHERDEVICE_H */
>
> --
> 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 PATCH 2/5] ethernet/intel: Use eth_skb_pad helper
From: Florian Fainelli @ 2014-11-26 1:05 UTC (permalink / raw)
To: Alexander Duyck, Eric Dumazet, Alexander Duyck
Cc: netdev, Jeff Kirsher, davem
In-Reply-To: <54752263.2040204@gmail.com>
On 25/11/14 16:44, Alexander Duyck wrote:
> On 11/25/2014 03:14 PM, Eric Dumazet wrote:
>> On Tue, 2014-11-25 at 14:44 -0800, Alexander Duyck wrote:
>>> Update the Intel Ethernet drivers to use eth_skb_pad() instead of doing
>>> their own implementations of the function.
>>>
>>> Also this cleans up two other spots where skb_pad was called but the length
>>> and tail pointers were being manipulated directly instead of just having
>>> the padding length added via __skb_put.
>>>
>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
>>> ---
>>> drivers/net/ethernet/intel/e1000/e1000_main.c | 8 ++------
>>> drivers/net/ethernet/intel/fm10k/fm10k_main.c | 11 +++--------
>>> drivers/net/ethernet/intel/igb/igb_main.c | 11 +++--------
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 11 +++--------
>>> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 11 +++--------
>>> 5 files changed, 14 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> index 24f3986..862d198 100644
>>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> @@ -3136,12 +3136,8 @@ static netdev_tx_t e1000_xmit_frame(struct sk_buff *skb,
>>> * packets may get corrupted during padding by HW.
>>> * To WA this issue, pad all small packets manually.
>>> */
>>> - if (skb->len < ETH_ZLEN) {
>>> - if (skb_pad(skb, ETH_ZLEN - skb->len))
>>> - return NETDEV_TX_OK;
>>> - skb->len = ETH_ZLEN;
>>> - skb_set_tail_pointer(skb, ETH_ZLEN);
>>> - }
>>> + if (eth_skb_pad(skb))
>>> + return NETDEV_TX_OK;
>>>
>> Its a bit sad almost no driver increments some drop counter.
>>
>> This probably could be generically done in eth_skb_pad()
>>
>> atomic_long_inc(&skb->dev->tx_dropped)
>
> The only problem is eth_skb_pad is called in the Rx path of some drivers
> as well.
>
> I wonder if we couldn't make this some sort of netdevice attribute to
> indicate what the smallest frame we can handle is and then just pad the
> frame to that as a part of __dev_xmit_skb. Then we could do that
> outside of the locks and take care of it before we even hit the qdisc layer.
One potential problem could be that the padding size varies at runtime
based on e.g: netdev features, connection to an Ethernet switch etc...
we could probably just advertise whatever maximum padding we need once
and for all and just assume that any skb we get called with in a
driver's xmit() has the required padding, that is probably fine too.
--
Florian
^ permalink raw reply
* Re: [PATCH] bonding: move ipoib_header_ops to vmlinux
From: Wengang @ 2014-11-26 1:30 UTC (permalink / raw)
To: David Miller, jay.vosburgh-Z7WLFzj8eWMS+FvcfC7Uqw
Cc: ogerlitz-VPRAkNaXOzVWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20141125.134450.1265438298771389292.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
于 2014年11月26日 02:44, David Miller 写道:
> From: Jay Vosburgh <jay.vosburgh-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Date: Tue, 25 Nov 2014 10:41:17 -0800
>
>> Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
>>
>>> On 11/25/2014 8:07 AM, David Miller wrote:
>>>> IPOIB should not work over bonding as it requires that the device
>>>> use ARPHRD_ETHER.
>>> Hi Dave,
>>>
>>> IPoIB devices can be enslaved to both bonding and teaming in their HA mode,
>>> the bond device type becomes ARPHRD_INFINIBAND when this happens.
>> The point was that pktgen disallows ARPHRD_INFINIBAND, not that
>> bonding does.
>>
>> Pktgen specifically checks for type != ARPHRD_ETHER, so the
>> IPoIB bond should not be able to be used with pkgten. My suspicion is
>> that pktgen is being configured on the bond first, then an IPoIB slave
>> is added to the bond; this would change its type in a way that pktgen
>> wouldn't notice.
> +1
I think it go this way:
1) bond_master is ready
2) bond_enslave enslave a IPOIB interface calling bond_setup_by_slave
3) then bond_setup_by_slave set change master type to ARPHRD_INFINIBAND.
code is like this:
1 /* enslave device <slave> to bond device <master> */
2 int bond_enslave(struct net_device *bond_dev, struct net_device
*slave_dev)
3 {
4 <snip>...
5 /* set bonding device ether type by slave - bonding netdevices are
6 * created with ether_setup, so when the slave type is not ARPHRD_ETHER
7 * there is a need to override some of the type dependent attribs/funcs.
8 *
9 * bond ether type mutual exclusion - don't allow slaves of dissimilar
10 * ether type (eg ARPHRD_ETHER and ARPHRD_INFINIBAND) share the same bond
11 */
12 if (!bond_has_slaves(bond)) {
13 if (bond_dev->type != slave_dev->type) {
14 <snip>...
15 if (slave_dev->type != ARPHRD_ETHER)
16 bond_setup_by_slave(bond_dev, slave_dev);
17 else {
18 ether_setup(bond_dev);
19 bond_dev->priv_flags &= ~IFF_TX_SKB_SHARING;
20 }
21
22 call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE,
23 bond_dev);
24 }
25 <snip>...
26 }
27
28 static void bond_setup_by_slave(struct net_device *bond_dev,
29 struct net_device *slave_dev)
30 {
31 bond_dev->header_ops = slave_dev->header_ops;
32
33 bond_dev->type = slave_dev->type;
34 bond_dev->hard_header_len = slave_dev->hard_header_len;
35 bond_dev->addr_len = slave_dev->addr_len;
36
37 memcpy(bond_dev->broadcast, slave_dev->broadcast,
38 slave_dev->addr_len);
39 }
40
thanks
wengang
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [patch net-next v3 08/17] bridge: call netdev_sw_port_stp_update when bridge port STP status changes
From: Scott Feldman @ 2014-11-26 1:35 UTC (permalink / raw)
To: Roopa Prabhu
Cc: Jiri Pirko, Netdev, David S. Miller, nhorman@tuxdriver.com,
Andy Gospodarek, Thomas Graf, dborkman@redhat.com,
ogerlitz@mellanox.com, jesse@nicira.com, pshelar@nicira.com,
azhou@nicira.com, ben@decadent.org.uk, stephen@networkplumber.org,
Kirsher, Jeffrey T, vyasevic@redhat.com, Cong Wang,
Fastabend, John R, Eric Dumazet, Jamal Hadi Salim,
Florian Fainelli, John Linville
In-Reply-To: <5475073E.70503@cumulusnetworks.com>
So offload is a little strong for this particular function. The
bridge driver or external STP process (msptd) is still controlling STP
state for the port and processing the BPDUs. When the state changes
on the port, the bridge driver is letting HW know, that's it. If the
port driver can't do anything with that notification, then it should
not implement ndo_switch_port_stp_update. If it does implement
ndo_switch_port_stp_update, then it can adjust its HW (e.g. disable
port if BR_DISABLED, etc), and return err code if somehow it failed
while adjusting HW.
This is not offloading STP state ctrl plane to HW. The ctrl plane is
kept in bridge driver (or mstpd) in SW. HW stays dumb in this model.
The bridge currently has policy control to turn on/off STP per bridge
and a netlink hook for external processes to change STP state.
On Tue, Nov 25, 2014 at 12:48 PM, Roopa Prabhu
<roopa@cumulusnetworks.com> wrote:
> On 11/25/14, 2:28 AM, Jiri Pirko wrote:
>>
>> From: Scott Feldman <sfeldma@gmail.com>
>>
>> To notify switch driver of change in STP state of bridge port, add new
>> .ndo op and provide switchdev wrapper func to call ndo op. Use it in
>> bridge
>> code then.
>>
>> Signed-off-by: Scott Feldman <sfeldma@gmail.com>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>> v2->v3:
>> -changed "sw" string to "switch" to avoid confusion
>> v1->v2:
>> -no change
>> ---
>> include/linux/netdevice.h | 5 +++++
>> include/net/switchdev.h | 7 +++++++
>> net/bridge/br_stp.c | 2 ++
>> net/switchdev/switchdev.c | 19 +++++++++++++++++++
>> 4 files changed, 33 insertions(+)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index ce096dc..66cb64e 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1024,6 +1024,9 @@ typedef u16 (*select_queue_fallback_t)(struct
>> net_device *dev,
>> * Called to get an ID of the switch chip this port is part of.
>> * If driver implements this, it indicates that it represents a port
>> * of a switch chip.
>> + * int (*ndo_switch_port_stp_update)(struct net_device *dev, u8 state);
>> + * Called to notify switch device port of bridge port STP
>> + * state change.
>> */
>> struct net_device_ops {
>> int (*ndo_init)(struct net_device *dev);
>> @@ -1180,6 +1183,8 @@ struct net_device_ops {
>> #ifdef CONFIG_NET_SWITCHDEV
>> int (*ndo_switch_parent_id_get)(struct
>> net_device *dev,
>> struct
>> netdev_phys_item_id *psid);
>> + int (*ndo_switch_port_stp_update)(struct
>> net_device *dev,
>> + u8 state);
>> #endif
>> };
>> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
>> index 7a52360..8a6d164 100644
>> --- a/include/net/switchdev.h
>> +++ b/include/net/switchdev.h
>> @@ -16,6 +16,7 @@
>> int netdev_switch_parent_id_get(struct net_device *dev,
>> struct netdev_phys_item_id *psid);
>> +int netdev_switch_port_stp_update(struct net_device *dev, u8 state);
>> #else
>> @@ -25,6 +26,12 @@ static inline int netdev_switch_parent_id_get(struct
>> net_device *dev,
>> return -EOPNOTSUPP;
>> }
>> +static inline int netdev_switch_port_stp_update(struct net_device *dev,
>> + u8 state)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>> +
>> #endif
>> #endif /* _LINUX_SWITCHDEV_H_ */
>> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
>> index 2b047bc..35e016c 100644
>> --- a/net/bridge/br_stp.c
>> +++ b/net/bridge/br_stp.c
>> @@ -12,6 +12,7 @@
>> */
>> #include <linux/kernel.h>
>> #include <linux/rculist.h>
>> +#include <net/switchdev.h>
>> #include "br_private.h"
>> #include "br_private_stp.h"
>> @@ -39,6 +40,7 @@ void br_log_state(const struct net_bridge_port *p)
>> void br_set_state(struct net_bridge_port *p, unsigned int state)
>> {
>> p->state = state;
>> + netdev_switch_port_stp_update(p->dev, state);
>> }
>> /* called under bridge lock */
>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> index 66973de..d162b21 100644
>> --- a/net/switchdev/switchdev.c
>> +++ b/net/switchdev/switchdev.c
>> @@ -31,3 +31,22 @@ int netdev_switch_parent_id_get(struct net_device *dev,
>> return ops->ndo_switch_parent_id_get(dev, psid);
>> }
>> EXPORT_SYMBOL(netdev_switch_parent_id_get);
>> +
>> +/**
>> + * netdev_switch_port_stp_update - Notify switch device port of STP
>> + * state change
>> + * @dev: port device
>> + * @state: port STP state
>> + *
>> + * Notify switch device port of bridge port STP state change.
>> + */
>> +int netdev_switch_port_stp_update(struct net_device *dev, u8 state)
>> +{
>> + const struct net_device_ops *ops = dev->netdev_ops;
>> +
>> + if (!ops->ndo_switch_port_stp_update)
>> + return -EOPNOTSUPP;
>> + WARN_ON(!ops->ndo_switch_parent_id_get);
>> + return ops->ndo_switch_port_stp_update(dev, state);
>> +}
>> +EXPORT_SYMBOL(netdev_switch_port_stp_update);
>
>
> This should also check if offload is enabled on the bridge/port ?
>
^ permalink raw reply
* Re: [net-next PATCH 2/5] ethernet/intel: Use eth_skb_pad helper
From: Eric Dumazet @ 2014-11-26 1:43 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Alexander Duyck, netdev, Jeff Kirsher, davem
In-Reply-To: <54752263.2040204@gmail.com>
On Tue, 2014-11-25 at 16:44 -0800, Alexander Duyck wrote:
>
> I wonder if we couldn't make this some sort of netdevice attribute to
> indicate what the smallest frame we can handle is and then just pad the
> frame to that as a part of __dev_xmit_skb. Then we could do that
> outside of the locks and take care of it before we even hit the qdisc layer.
Well, many NIC do not have such restriction.
Do we have some kind of counters of skb->head reallocations caused by
this padding ?
I believe I finally have an idea why we had various + 15 in skb
allocations in TCP stack !
We probably should reinstate them, as ACK packets can be 54 bytes long.
^ 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