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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200304102705.192d3b0a@kicinski-fedora-PC1C0HJN>
Date:   Wed, 4 Mar 2020 10:27:05 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alexander Duyck <alexander.h.duyck@...ux.intel.com>
Cc:     Michal Kubecek <mkubecek@...e.cz>, davem@...emloft.net,
        thomas.lendacky@....com, benve@...co.com, _govind@....com,
        pkaustub@...co.com, peppe.cavallaro@...com,
        alexandre.torgue@...com, joabreu@...opsys.com, snelson@...sando.io,
        yisen.zhuang@...wei.com, salil.mehta@...wei.com,
        jeffrey.t.kirsher@...el.com, jacob.e.keller@...el.com,
        michael.chan@...adcom.com, saeedm@...lanox.com, leon@...nel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for
 centralized checking of coalescing parameters

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 🤔

> > > > +	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.