From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 18CBBC3F2D1 for ; Wed, 4 Mar 2020 18:51:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EBEAC20732 for ; Wed, 4 Mar 2020 18:51:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730176AbgCDSvF (ORCPT ); Wed, 4 Mar 2020 13:51:05 -0500 Received: from mga09.intel.com ([134.134.136.24]:13652 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729600AbgCDSvE (ORCPT ); Wed, 4 Mar 2020 13:51:04 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Mar 2020 10:50:57 -0800 X-IronPort-AV: E=Sophos;i="5.70,514,1574150400"; d="scan'208";a="240551018" Received: from ahduyck-desk1.jf.intel.com ([10.7.198.76]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Mar 2020 10:50:57 -0800 Message-ID: <443c085ff3bcc06ad15c34f44f0407a0861dadeb.camel@linux.intel.com> Subject: Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters From: Alexander Duyck To: Jakub Kicinski Cc: Michal Kubecek , 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 Date: Wed, 04 Mar 2020 10:50:57 -0800 In-Reply-To: <20200304102705.192d3b0a@kicinski-fedora-PC1C0HJN> References: <20200304043354.716290-1-kuba@kernel.org> <20200304043354.716290-2-kuba@kernel.org> <20200304075926.GH4264@unicorn.suse.cz> <20200304100050.14a95c36@kicinski-fedora-PC1C0HJN> <45b3c493c3ce4aa79f882a8170f3420d348bb61e.camel@linux.intel.com> <20200304102705.192d3b0a@kicinski-fedora-PC1C0HJN> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.32.5 (3.32.5-1.fc30) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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.