| Message ID | 20210825083832.2425886-1-dqfext@gmail.com |
|---|---|
| Headers | show |
| Series | DSA slave with customise netdev features | expand |
On Wed, Aug 25, 2021 at 04:38:29PM +0800, DENG Qingfang wrote: > Some taggers, such as tag_dsa.c, combine VLAN tags with DSA tags, which > currently has a few problems: > > 1. Unnecessary reallocation on TX: > > A central TX reallocation has been used since commit a3b0b6479700 > ("net: dsa: implement a central TX reallocation procedure"), but for > VLAN-tagged frames, the actual headroom required for DSA taggers which > combine with VLAN tags is smaller. If true, this would be a major failure of the central TX reallocation idea. However, for this to fail, it would mean that there is a code path in the network stack that routinely allocates skbs with skb_needed_headroom(skb) smaller than dev->needed_headroom. That's the only thing that would trigger reallocs (beside cloned skbs). The fact that we ask for a dev->needed_headroom that is a bit larger than what is needed in some cases is not an issue. We should declare the largest dev->needed_headroom that should cover all cases. Would you please let me know what is the stack trace when you apply this patch? -----------------------------[ cut here ]----------------------------- diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 6d6f1aebf1ca..1924025ac136 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -600,6 +600,10 @@ static int dsa_realloc_skb(struct sk_buff *skb, struct net_device *dev) /* No reallocation needed, yay! */ return 0; + netdev_err(dev, "%s: skb realloc: headroom %u, tailroom %u, cloned %d, needed_headroom %d, needed_tailroom %d\n", + __func__, skb_headroom(skb), skb_tailroom(skb), skb_cloned(skb), needed_headroom, needed_tailroom); + WARN_ON(!skb_cloned(skb)); + return pskb_expand_head(skb, needed_headroom, needed_tailroom, GFP_ATOMIC); } -----------------------------[ cut here ]----------------------------- > > 2. Repeated memmoves: > > If a both Marvell EDSA and VLAN tagged frame is received, the current > code will move the (DA,SA) twice: the first in dsa_rcv_ll to convert the > frame to a normal 802.1Q frame, and the second to strip off the 802.1Q > tag. The similar thing happens on TX. I don't think a lot of people use 8021q uppers with mv88e6xxx. At least the error messages I've seen during the few times when I've booted the Turris Mox would seem to suggest that. The code that converts DSA 'tagged' frames to VLAN tags originates from Lennert Buytenhek himself. > > For these tags, it is better to handle DSA and VLAN tags at the same time > in DSA taggers. No objection there. > > This patch set allows taggers to add custom netdev features to DSA > slaves so they can advertise VLAN offload, and converts tag_mtk to use > the TX VLAN offload. > > DENG Qingfang (2): > net: dsa: allow taggers to customise netdev features > net: dsa: tag_mtk: handle VLAN tag insertion on TX > > include/net/dsa.h | 2 ++ > net/dsa/slave.c | 3 ++- > net/dsa/tag_mtk.c | 46 ++++++++++++++++++++++------------------------ > 3 files changed, 26 insertions(+), 25 deletions(-) > > -- > 2.25.1 >
On Wed, Aug 25, 2021 at 04:38:30PM +0800, DENG Qingfang wrote: > Allow taggers to add netdev features, such as VLAN offload, to slave > devices, which will make it possible for taggers to handle VLAN tags > themselves. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
On 8/25/2021 10:38 AM, DENG Qingfang wrote: > Allow taggers to add netdev features, such as VLAN offload, to slave > devices, which will make it possible for taggers to handle VLAN tags > themselves. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> Besides transmit VLAN tag offload, do you think there are other netdev feature bits that would warrant something like this as opposed to a more structured approach with adding a specific boolean/flag that is specific to the netdev feature you want to propagate towards the DSA slave_dev? Still: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
Some taggers, such as tag_dsa.c, combine VLAN tags with DSA tags, which currently has a few problems: 1. Unnecessary reallocation on TX: A central TX reallocation has been used since commit a3b0b6479700 ("net: dsa: implement a central TX reallocation procedure"), but for VLAN-tagged frames, the actual headroom required for DSA taggers which combine with VLAN tags is smaller. 2. Repeated memmoves: If a both Marvell EDSA and VLAN tagged frame is received, the current code will move the (DA,SA) twice: the first in dsa_rcv_ll to convert the frame to a normal 802.1Q frame, and the second to strip off the 802.1Q tag. The similar thing happens on TX. For these tags, it is better to handle DSA and VLAN tags at the same time in DSA taggers. This patch set allows taggers to add custom netdev features to DSA slaves so they can advertise VLAN offload, and converts tag_mtk to use the TX VLAN offload. DENG Qingfang (2): net: dsa: allow taggers to customise netdev features net: dsa: tag_mtk: handle VLAN tag insertion on TX include/net/dsa.h | 2 ++ net/dsa/slave.c | 3 ++- net/dsa/tag_mtk.c | 46 ++++++++++++++++++++++------------------------ 3 files changed, 26 insertions(+), 25 deletions(-)