Re: [net-next 13/15] ice: Implement ethtool ops for channels

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Thu, 21 Nov 2019 23:09:06 -0800, Jeff Kirsher wrote:
> On Thu, 2019-11-21 at 14:42 -0800, Jakub Kicinski wrote:
> > On Wed, 20 Nov 2019 23:46:10 -0800, Jeff Kirsher wrote:  
> > > +	curr_combined = ice_get_combined_cnt(vsi);
> > > +
> > > +	/* these checks are for cases where user didn't specify a
> > > particular
> > > +	 * value on cmd line but we get non-zero value anyway via
> > > +	 * get_channels(); look at ethtool.c in ethtool repository (the
> > > user
> > > +	 * space part), particularly, do_schannels() routine
> > > +	 */
> > > +	if (ch->rx_count == vsi->num_rxq - curr_combined)
> > > +		ch->rx_count = 0;
> > > +	if (ch->tx_count == vsi->num_txq - curr_combined)
> > > +		ch->tx_count = 0;
> > > +	if (ch->combined_count == curr_combined)
> > > +		ch->combined_count = 0;
> > > +
> > > +	if (!(ch->combined_count || (ch->rx_count && ch->tx_count))) {
> > > +		netdev_err(dev, "Please specify at least 1 Rx and 1 Tx
> > > channel\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	new_rx = ch->combined_count + ch->rx_count;
> > > +	new_tx = ch->combined_count + ch->tx_count;  
> > 
> > The combined vs individual count logic looks correct to me which is not
> > common, so nice to see that!
> >   
> > > +	if (new_rx > ice_get_max_rxq(pf)) {
> > > +		netdev_err(dev, "Maximum allowed Rx channels is %d\n",
> > > +			   ice_get_max_rxq(pf));
> > > +		return -EINVAL;
> > > +	}
> > > +	if (new_tx > ice_get_max_txq(pf)) {
> > > +		netdev_err(dev, "Maximum allowed Tx channels is %d\n",
> > > +			   ice_get_max_txq(pf));
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ice_vsi_recfg_qs(vsi, new_rx, new_tx);
> > > +
> > > +	if (new_rx)
> > > +		return ice_vsi_set_dflt_rss_lut(vsi, new_rx);  
> > 
> > But I don't see you doing a netif_is_rxfh_configured() check, which is
> > supposed to prevent reconfiguring the RSS indirection table if it was
> > set up manually by the user.  
> 
> Talking with Henry it appears Jake's kernel changes were done in parallel
> or after the work that was done in the driver.  

Really?

commit d4ab4286276fcd6c155bafdf4422b712068d2516
Author: Keller, Jacob E <jacob.e.keller@intel.com>
Date:   Mon Feb 8 16:05:03 2016 -0800
            ^^^^^          ^^^^
    ethtool: correctly ensure {GS}CHANNELS doesn't conflict with GS{RXFH}

> There appears to be more
> code that just adding a simple check, is this a *required* change for
> acceptance.  We are currently looking into making this fix, but due to the
> upcoming holidays, not sure if we can make the desired change before the
> merge window closes.  So I see the following options:
> 
> - accept patch as is, and expect a fix in the next patch submission
> - drop this patch since the fix will not be available before the merge
> window closes for net-next
> 
> While I prefer the first option because we are also submitting the "new"
> virtio_bus patches, which depend heavily on these changes upstream, I think
> we can drop this patch (if necessary) and be fine to submit the other
> patches with little thrash.
> 
> Which option is preferred or acceptable to you Dave and Jakub?

I don't feel very strongly about this, but what if 5.5 gets picked 
by some distro and users actually start depending on the incorrect
behaviour? :(

I'm a little worried that the fix is not trivial, I think it should be.
Is the effort in validation?

I'd think best way forward would be to drop this patch, get the rest of
the series in, and since this one patch was initially posted before the
merge window started maybe you could persuade Dave to merge it after
net-next closes (but before -rc1).

Also, perhaps some Central Europeans can help out if it's Thanksgiving
getting in the way? Not sure that'd work for your processes..