RE: [EXTERNAL] Re: [v2 net PATCH] octeontx2-pf: Disable HW TSO for seg size < 16B

[Geethasowjanya Akula]

> -----Original Message-----
> From: Eric Dumazet <edumazet@xxxxxxxxxx>
> Sent: Wednesday, March 13, 2024 1:28 PM
> To: Geethasowjanya Akula <gakula@xxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> davem@xxxxxxxxxxxxx; pabeni@xxxxxxxxxx; Sunil Kovvuri Goutham
> <sgoutham@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta
> <sbhatta@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>
> Subject: [EXTERNAL] Re: [v2 net PATCH] octeontx2-pf: Disable HW TSO for seg
> size < 16B
> On Wed, Mar 13, 2024 at 7:33 AM Geetha sowjanya <gakula@xxxxxxxxxxx>
> wrote:
> >
> > Current NIX hardware do not support TSO for the segment size less 16
> > bytes. This patch disable hw TSO for such packets.
> >
> > Fixes: 86d7476078b8 ("octeontx2-pf: TCP segmentation offload support").
> > Signed-off-by: Geetha sowjanya <gakula@xxxxxxxxxxx>
> > ---
> >
> > v1-v2:
> >  - As suggested by Eric Dumazet used ndo_features_check().
> >  - Moved the max fargments support check to ndo_features_check.
> >
> >  .../marvell/octeontx2/nic/otx2_common.c        | 18 ++++++++++++++++++
> >  .../marvell/octeontx2/nic/otx2_common.h        |  3 +++
> >  .../ethernet/marvell/octeontx2/nic/otx2_pf.c   |  1 +
> >  .../ethernet/marvell/octeontx2/nic/otx2_txrx.c | 11 -----------
> >  .../ethernet/marvell/octeontx2/nic/otx2_vf.c   |  1 +
> >  5 files changed, 23 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > index 02d0b707aea5..de61c69370be 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > @@ -221,6 +221,24 @@ int otx2_set_mac_address(struct net_device
> > *netdev, void *p)  }  EXPORT_SYMBOL(otx2_set_mac_address);
> >
> > +netdev_features_t
> > +otx2_features_check(struct sk_buff *skb, struct net_device *dev,
> > +                   netdev_features_t features) {
> > +       /* Due to hw issue segment size less than 16 bytes
> > +        * are not supported. Hence do not offload such TSO
> > +        * segments.
> > +        */
> > +       if (skb_is_gso(skb) && skb_shinfo(skb)->gso_size < 16)
> > +               features &= ~NETIF_F_GSO_MASK;
> > +
> > +       if (skb_shinfo(skb)->nr_frags + 1 > OTX2_MAX_FRAGS_IN_SQE)
> > +               features &= ~NETIF_F_SG;
> > +
> 
> This is a bit extreme. I would attempt to remove NETIF_F_GSO_MASK instead.
> 
> if (skb_is_gso(skb)) {
>      if (skb_shinfo(skb)->gso_size < 16 ||
>          skb_shinfo(skb)->nr_frags + 1 > OTX2_MAX_FRAGS_IN_SQE))
>            features &= ~NETIF_F_GSO_MASK; }
> 
> This would very often end up with 1-MSS packets with fewer fragments.
> 
> -> No copy involved, and no high order page allocations.
> 
> > +       return features;
> > +}
> > +EXPORT_SYMBOL(otx2_features_check);
> > +
> >  int otx2_hw_set_mtu(struct otx2_nic *pfvf, int mtu)  {
> >         struct nix_frs_cfg *req;
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > index 06910307085e..6a4bf43bc77e 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > @@ -961,6 +961,9 @@ void otx2_get_mac_from_af(struct net_device
> > *netdev);  void otx2_config_irq_coalescing(struct otx2_nic *pfvf, int
> > qidx);  int otx2_config_pause_frm(struct otx2_nic *pfvf);  void
> > otx2_setup_segmentation(struct otx2_nic *pfvf);
> > +netdev_features_t otx2_features_check(struct sk_buff *skb,
> > +                                     struct net_device *dev,
> > +                                     netdev_features_t features);
> >
> >  /* RVU block related APIs */
> >  int otx2_attach_npa_nix(struct otx2_nic *pfvf); diff --git
> > a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > index e5fe67e73865..2364eb8d6732 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > @@ -2737,6 +2737,7 @@ static const struct net_device_ops
> otx2_netdev_ops = {
> >         .ndo_xdp_xmit           = otx2_xdp_xmit,
> >         .ndo_setup_tc           = otx2_setup_tc,
> >         .ndo_set_vf_trust       = otx2_ndo_set_vf_trust,
> > +       .ndo_features_check     = otx2_features_check,
> >  };
> >
> >  static int otx2_wq_init(struct otx2_nic *pf) diff --git
> > a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > index f828d32737af..9b89aff42eb0 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> > @@ -1158,17 +1158,6 @@ bool otx2_sq_append_skb(struct net_device
> > *netdev, struct otx2_snd_queue *sq,
> >
> >         num_segs = skb_shinfo(skb)->nr_frags + 1;
> >
> > -       /* If SKB doesn't fit in a single SQE, linearize it.
> > -        * TODO: Consider adding JUMP descriptor instead.
> > -        */
> > -       if (unlikely(num_segs > OTX2_MAX_FRAGS_IN_SQE)) {
> > -               if (__skb_linearize(skb)) {
> > -                       dev_kfree_skb_any(skb);
> > -                       return true;
> > -               }
> > -               num_segs = skb_shinfo(skb)->nr_frags + 1;
> > -       }
> 
> Then you need to keep this check for  non GSO packets.
> 
> One way to trigger this is to run netperf with tiny fragments.
> TCP is unable to cook GSO packets, because we hit MAX_SKB_FRAGS before
> even filling a single MSS.
> 
> netperf -H $remote -t TCP_SENDFILE -- -m 10
> 
Will repost the patch with suggested changes and testing.
> 
> 
> > -
> >         if (skb_shinfo(skb)->gso_size && !is_hw_tso_supported(pfvf, skb)) {
> >                 /* Insert vlan tag before giving pkt to tso */
> >                 if (skb_vlan_tag_present(skb)) diff --git
> > a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> > index 35e06048356f..04aab04e4ba2 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
> > @@ -483,6 +483,7 @@ static const struct net_device_ops
> otx2vf_netdev_ops = {
> >         .ndo_tx_timeout = otx2_tx_timeout,
> >         .ndo_eth_ioctl  = otx2_ioctl,
> >         .ndo_setup_tc = otx2_setup_tc,
> > +       .ndo_features_check     = otx2_features_check,
> >  };
> >
> >  static int otx2_wq_init(struct otx2_nic *vf)
> > --
> > 2.25.1
> >