Re: [net-next PATCH] octeontx2-pf: Change receive buffer size using ethtool

[sundeep subbaraya <sundeep.lkml@gmail.com>]

Hi Jakub,

On Thu, Jan 13, 2022 at 5:24 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 12 Jan 2022 22:32:55 +0530 Subbaraya Sundeep wrote:
> > ethtool rx-buf-len is for setting receive buffer size,
> > support setting it via ethtool -G parameter and getting
> > it via ethtool -g parameter.
>
> I don't see a check against current MTU, in case MTU is larger than
> the buffer length the device will scatter?
>
Yes correct. The idea is to have an option for user to choose either one big
frame or multiple fragments for a frame.

> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > index 61e5281..6d11cb2 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> > @@ -177,6 +177,7 @@ struct otx2_hw {
> >       u16                     pool_cnt;
> >       u16                     rqpool_cnt;
> >       u16                     sqpool_cnt;
> > +     u16                     rbuf_len;
> >
> >       /* NPA */
> >       u32                     stack_pg_ptrs;  /* No of ptrs per stack page */
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> > index d85db90..a100296 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ethtool.c
> > @@ -371,6 +371,7 @@ static void otx2_get_ringparam(struct net_device *netdev,
> >       ring->rx_pending = qs->rqe_cnt ? qs->rqe_cnt : Q_COUNT(Q_SIZE_256);
> >       ring->tx_max_pending = Q_COUNT(Q_SIZE_MAX);
> >       ring->tx_pending = qs->sqe_cnt ? qs->sqe_cnt : Q_COUNT(Q_SIZE_4K);
> > +     kernel_ring->rx_buf_len = pfvf->hw.rbuf_len;
> >  }
> >
> >  static int otx2_set_ringparam(struct net_device *netdev,
> > @@ -379,6 +380,7 @@ static int otx2_set_ringparam(struct net_device *netdev,
> >                             struct netlink_ext_ack *extack)
> >  {
> >       struct otx2_nic *pfvf = netdev_priv(netdev);
> > +     u32 rx_buf_len = kernel_ring->rx_buf_len;
> >       bool if_up = netif_running(netdev);
> >       struct otx2_qset *qs = &pfvf->qset;
> >       u32 rx_count, tx_count;
> > @@ -386,6 +388,15 @@ static int otx2_set_ringparam(struct net_device *netdev,
> >       if (ring->rx_mini_pending || ring->rx_jumbo_pending)
> >               return -EINVAL;
> >
> > +     /* Hardware supports max size of 32k for a receive buffer
> > +      * and 1536 is typical ethernet frame size.
> > +      */
> > +     if (rx_buf_len && (rx_buf_len < 1536 || rx_buf_len > 32768)) {
> > +             netdev_err(netdev,
> > +                        "Receive buffer range is 1536 - 32768");
> > +             return -EINVAL;
> > +     }
> > +
> >       /* Permitted lengths are 16 64 256 1K 4K 16K 64K 256K 1M  */
> >       rx_count = ring->rx_pending;
> >       /* On some silicon variants a skid or reserved CQEs are
> > @@ -403,7 +414,7 @@ static int otx2_set_ringparam(struct net_device *netdev,
> >                          Q_COUNT(Q_SIZE_4K), Q_COUNT(Q_SIZE_MAX));
> >       tx_count = Q_COUNT(Q_SIZE(tx_count, 3));
> >
> > -     if (tx_count == qs->sqe_cnt && rx_count == qs->rqe_cnt)
> > +     if (tx_count == qs->sqe_cnt && rx_count == qs->rqe_cnt && !rx_buf_len)
>
> Should we use rx_buf_len = 0 as a way for users to reset the rxbuf len
> to the default value? I think that would be handy.
>
Before this patch we calculate each receive buffer based on mtu set by user.
Now user can use rx-buf-len but the old mtu based calculation is also there.
Here I am using rx_buf_len == 0 as a switch to calculate buffer length
using mtu or
just use length set by user. So here I am not setting rx_buf_len to some
default value.

> >       if (if_up)
> > @@ -413,6 +424,10 @@ static int otx2_set_ringparam(struct net_device *netdev,
> >       qs->sqe_cnt = tx_count;
> >       qs->rqe_cnt = rx_count;
> >
> > +     if (rx_buf_len)
> > +             pfvf->hw.rbuf_len = ALIGN(rx_buf_len, OTX2_ALIGN) +
> > +                                 OTX2_HEAD_ROOM;
> > +
> >       if (if_up)
> >               return netdev->netdev_ops->ndo_open(netdev);
> >
> > @@ -1207,6 +1222,7 @@ static int otx2_set_link_ksettings(struct net_device *netdev,
> >  static const struct ethtool_ops otx2_ethtool_ops = {
> >       .supported_coalesce_params = ETHTOOL_COALESCE_USECS |
> >                                    ETHTOOL_COALESCE_MAX_FRAMES,
> > +     .supported_ring_params  = ETHTOOL_RING_USE_RX_BUF_LEN,
> >       .get_link               = otx2_get_link,
> >       .get_drvinfo            = otx2_get_drvinfo,
> >       .get_strings            = otx2_get_strings,
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > index 6080ebd..37afffa 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
> > @@ -66,6 +66,8 @@ static int otx2_change_mtu(struct net_device *netdev, int new_mtu)
> >                   netdev->mtu, new_mtu);
> >       netdev->mtu = new_mtu;
> >
> > +     pf->hw.rbuf_len = 0;
>
> Why reset the buf len on mtu change?
>
As explained above buffer size will be calculated using mtu
now instead of rx-buf-len from ethtool.

Thanks,
Sundeep

> >       if (if_up)
> >               err = otx2_open(netdev);
> >
> > @@ -1306,6 +1308,9 @@ static int otx2_get_rbuf_size(struct otx2_nic *pf, int mtu)
> >       int total_size;
> >       int rbuf_size;
> >
> > +     if (pf->hw.rbuf_len)
> > +             return pf->hw.rbuf_len;
> > +
> >       /* The data transferred by NIX to memory consists of actual packet
> >        * plus additional data which has timestamp and/or EDSA/HIGIG2
> >        * headers if interface is configured in corresponding modes.
>