Re: [PATCH net-next v3 2/7] bnxt_en: add support for tcp-data-split ethtool command

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 9 Oct 2024 22:54:17 +0900 Taehee Yoo wrote:
> > This breaks previous behavior. The HDS reporting from get was
> > introduced to signal to user space whether the page flip based
> > TCP zero-copy (the one added some years ago not the recent one)
> > will be usable with this NIC.
> >
> > When HW-GRO is enabled HDS will be working.
> >
> > I think that the driver should only track if the user has set the value
> > to ENABLED (forced HDS), or to UKNOWN (driver default). Setting the HDS
> > to disabled is not useful, don't support it.  
> 
> Okay, I will remove the disable feature in a v4 patch.
> Before this patch, hds_threshold was rx-copybreak value.
> How do you think hds_threshold should still follow rx-copybreak value
> if it is UNKNOWN mode?

IIUC the rx_copybreak only applies to the header? Or does it apply 
to the entire frame?

If rx_copybreak applies to the entire frame and not just the first
buffer (headers or headers+payload if not split) - no preference.
If rx_copybreak only applies to the headers / first buffer then
I'd keep them separate as they operate on a different length.

> I think hds_threshold need to follow new tcp-data-split-thresh value in
> ENABLE/UNKNOWN and make rx-copybreak pure software feature.

Sounds good to me, but just to be clear:

If user sets the HDS enable to UNKNOWN (or doesn't set it):
 - GET returns (current behavior, AFAIU):
   - DISABLED (if HW-GRO is disabled and MTU is not Jumbo)
   - ENABLED (if HW-GRO is enabled of MTU is Jumbo)
If user sets the HDS enable to ENABLED (force HDS on):
 - GET returns ENABLED 

hds_threshold returns: some value, but it's only actually used if GET
returns ENABLED.

> But if so, it changes the default behavior.

How so? The configuration of neither of those two is exposed to 
the user. We can keep the same defaults, until user overrides them.

> How do you think about it?
> 
> >  
> > >       ering->tx_max_pending = BNXT_MAX_TX_DESC_CNT;
> > >
> > >       ering->rx_pending = bp->rx_ring_size;
> > > @@ -854,9 +858,25 @@ static int bnxt_set_ringparam(struct net_device *dev,
> > >           (ering->tx_pending < BNXT_MIN_TX_DESC_CNT))
> > >               return -EINVAL;
> > >
> > > +     if (kernel_ering->tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_DISABLED &&
> > > +         BNXT_RX_PAGE_MODE(bp)) {
> > > +             NL_SET_ERR_MSG_MOD(extack, "tcp-data-split can not be enabled with XDP");
> > > +             return -EINVAL;
> > > +     }  
> >
> > Technically just if the XDP does not support multi-buffer.
> > Any chance we could do this check in the core?  
> 
> I think we can access xdp_rxq_info with netdev_rx_queue structure.
> However, xdp_rxq_info is not sufficient to distinguish mb is supported
> by the driver or not. I think prog->aux->xdp_has_frags is required to
> distinguish it correctly.
> So, I think we need something more.
> Do you have any idea?

Take a look at dev_xdp_prog_count(), something like that but only
counting non-mb progs?