Date:   Tue, 25 Jan 2022 04:15:23 -0300
From:   Luiz Angelo Daros de Luca <luizluca@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Alvin Šipraga <ALSI@...g-olufsen.dk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v4 11/11] net: dsa: realtek: rtl8365mb: multiple
 cpu ports, non cpu extint

Wow... that's a lot to digest.

> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  .../files/drivers/net/ethernet/ralink/mtk_eth_soc.c  | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c b/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c
> index e07e5ed5a8f8..6ed9bc5942fd 100644
> --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c
> +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/mtk_eth_soc.c
> @@ -31,6 +31,7 @@
>  #include <linux/io.h>
>  #include <linux/bug.h>
>  #include <linux/netfilter.h>
> +#include <net/dsa.h>
>  #include <net/netfilter/nf_flow_table.h>
>  #include <linux/of_gpio.h>
>  #include <linux/gpio.h>
> @@ -1497,6 +1498,16 @@ static int fe_change_mtu(struct net_device *dev, int new_mtu)
>         return fe_open(dev);
>  }
>
> +static netdev_features_t fe_features_check(struct sk_buff *skb,
> +                                          struct net_device *dev,
> +                                          netdev_features_t features)
> +{
> +       if (netdev_uses_dsa(dev))
> +               features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
> +
> +       return features;
> +}
> +
>  static const struct net_device_ops fe_netdev_ops = {
>         .ndo_init               = fe_init,
>         .ndo_uninit             = fe_uninit,
> @@ -1514,6 +1525,7 @@ static const struct net_device_ops fe_netdev_ops = {
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>         .ndo_poll_controller    = fe_poll_controller,
>  #endif
> +       .ndo_features_check     = fe_features_check,
>  };
>
>  static void fe_reset_pending(struct fe_priv *priv)

Thanks, Vladimir. I'll try that patch soon. However, it will never be
accepted even in OpenWrt as is because it does offload its own
proprietary tag.
I might need to add another if like:

> +       if (netdev_uses_dsa(dev))
> +               if (skb->???->proto_in_use != DSA_TAG_PROTO_MTK)
> +                      features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);

I think it would need to save the used tag in some form of an oob
signal (as Florian suggested). In that case, even the
netdev_uses_dsa() test can be removed as a skb with an oob tag signal
is surely from a dsa device.

Anyway, even with existing offload code not doing exactly what they
should, they normally work given a normal device usage. The strange
part is that DSA assumes those features, copied from master to slave,
will still be the same even after a tag was injected into the packet.

Sorry for my arrogance being a newbie but I think the place to fix the
problem is still in slave feature list. It is better than having the
kernel repeat the test for every single packet. And nobody will be
willing to add extra overhead to a working code just because DSA needs
it. It is easier to add a new function that does not touch existing
code paths.

I believe that those drivers with NETIF_F_HW_CSUM are fine for every
type of DSA, right? So, just those with NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM set needs to be adjusted. A fully implemented
ndo_features_check() will work but improving it for every driver will
add extra code/overhead for all packets, used with DSA or not. And
that extra code needed for DSA will either always keep or remove the
same features for a given slave.

I imagine that for NETIF_F_CSUM_MASK and NETIF_F_GSO_MASK, it would
not be too hard to build a set of candidate packets to test if that
feature is still valid after the tag was added. With that assumption,
a new ndo_features_check_offline(), similar to ndo_features_check()
but not be called by netif_skb_features, will test each candidate
during slave setup. If the check disagrees after the tag was added,
that feature should be disabled for that slave. Something like:

slave->features = master->features;
if (slave->features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM))
    if (dev->netdev_ops->ndo_features_check_offline)
        foreach (test_candidate)
            tagged_test_candidate = add_tag (test_candidate, slave->tag);

            slave->features &=
~(master->netdev_ops->ndo_features_check_offline(test_candidate,
master, slave->features) ^

master->netdev_ops->ndo_features_check_offline(tagged_test_candidate,
master, slave->features)
    else
        slave->features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK)

The only drivers that would have performance regression while used as
DSA master ports are those:
1) that does not have NETIF_F_HW_CSUM set
2) but could still offload after a particular DSA tag was added (when
tag vendor and HW matches)
3) and still didn't implement the new ndo_features_check_offline().

ndo_features_check_offline() would not be too much different from what
Vladmir suggested for the out-of-tree mtk_eth_soc driver.

ndo_features_check_offline(sbk, dev, features) {
    switch (sbk->oob->tag) {
    case SUPPORTED_TAG_1:
    case SUPPORTED_TAG_2:
    case SUPPORTED_TAG_3:
    case NO_TAG:
        break;
    default:
        features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
    }

    if (dev->netdev_ops->ndo_features_check)
         features &= dev->netdev_ops->ndo_features_check(skb, dev, features);

    /* some more test if needed*/

    return features;
}

If used exclusively by DSA, ndo_features_check_offline could also be
called ndo_dsa_features_check (or any better name than
ndo_features_check_offline). That is not far away from what Vladmir
suggested (and later retracted) in the first place.

Regards,

Luiz

Powered by blists - more mailing lists