Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Michal Kubecek <mkubecek@suse.cz>,
	davem@davemloft.net, thomas.lendacky@amd.com, benve@cisco.com,
	_govind@gmx.com, pkaustub@cisco.com, peppe.cavallaro@st.com,
	alexandre.torgue@st.com, joabreu@synopsys.com,
	snelson@pensando.io, yisen.zhuang@huawei.com,
	salil.mehta@huawei.com, jeffrey.t.kirsher@intel.com,
	jacob.e.keller@intel.com, michael.chan@broadcom.com,
	saeedm@mellanox.com, leon@kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters
Date: Wed, 04 Mar 2020 10:50:57 -0800	[thread overview]
Message-ID: <443c085ff3bcc06ad15c34f44f0407a0861dadeb.camel@linux.intel.com> (raw)
In-Reply-To: <20200304102705.192d3b0a@kicinski-fedora-PC1C0HJN>

On Wed, 2020-03-04 at 10:27 -0800, Jakub Kicinski wrote:
> On Wed, 04 Mar 2020 10:12:30 -0800 Alexander Duyck wrote:
> > On Wed, 2020-03-04 at 10:00 -0800, Jakub Kicinski wrote:
> > > On Wed, 4 Mar 2020 08:59:26 +0100 Michal Kubecek wrote:  
> > > > Just an idea: perhaps we could use the fact that struct ethtool_coalesce
> > > > is de facto an array so that this block could be replaced by a loop like
> > > > 
> > > > 	u32 supported_types = dev->ethtool_ops->coalesce_types;
> > > > 	const u32 *values = &coalesce->rx_coalesce_usecs;
> > > > 
> > > > 	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
> > > > 		if (values[i] && !(supported_types & BIT(i)))
> > > > 			return false;
> > > > 
> > > > and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
> > > > of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.  
> > > 
> > > I kind of prefer the greppability over the saved 40 lines :(
> > > But I'm happy to change if we get more votes for the more concise
> > > version. Or perhaps the Intel version with the warnings printed.  
> > 
> > I agree that it would make more sense to replace the types with an enum
> > definition, and then use the enum to define bits to be used by the
> > drivers.
> 
> The only use for the enum would then be to automate the bit assignment?
> Sounds like we would save some lines for the code and added some for
> the definition. Maybe I'm missing the advantage the enum brings 🤔

Well if you wanted to you could probably also update ethtool_coalesce to
support a unioned __u32 array if you wanted to be more explicit about it. 

I just figured that by making in an enum it becomes less error prone since
you can't accidentally leave a gap or end up renumbering things
unintentionally. Combine that with some logic to take care of the bit
shifting and it wouldn't differ much from how we handle the netdev feature
flags and the like.

> > > > > +	return !dev->ethtool_ops->coalesce_types ||
> > > > > +		(dev->ethtool_ops->coalesce_types & used_types) == used_types;
> > > > > +}    
> > > > 
> > > > I suggest to move the check for !dev->ethtool_ops->coalesce_types to the
> > > > beginning of the function so that we avoid calculating the bitmap if we
> > > > are not going to check it anyway.  
> > > 
> > > Good point!  
> > 
> > So one thing I just wanted to point out. The used_types won't necessarily
> > be correct because it is only actually checking for non-zero types. There
> > are some of these values where a zero is a valid input and the driver will
> > accept it, such as rx_coalesce_usecs for ixgbe. As such we might want to
> > rename the value to nonzero_types instead of used_types.
> 
> Okay, I'll rename. I was also wondering if it should be "params" not
> "types". Initially I was hoping there are categories of coalescing that
> drivers implement, each with set of params. But it seems each vendor is
> just picking fields they like. I think I'll do s/types/params/ as well.

Makes sense to me.