Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <48fe0346-40f9-b0f5-991c-8c17349849f7@intel.com>
Date:   Thu, 14 Sep 2023 16:10:48 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] net/core: Export dev_core_stats_rx_dropped_inc sets
Content-Language: en-US
To:     Yajun Deng <yajun.deng@linux.dev>
CC:     <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>, <horms@kernel.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20230911082016.3694700-1-yajun.deng@linux.dev>
 <9a1de9b3-b3cf-d26b-388e-c98294580bca@intel.com>
 <599cdff8-4865-3ade-0439-36e337891ca0@linux.dev>
 <e6de7c0b-97cc-090e-a331-dc566019f54e@intel.com>
 <dfe6cf9b-7cf9-f938-8ab1-a2d4c7741c64@linux.dev>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <dfe6cf9b-7cf9-f938-8ab1-a2d4c7741c64@linux.dev>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZUVLZXh2QzJab1ZHTmp5S2dQVU5FYmQza2hDUy90R0JqekkzS3IvK2hsSjJI?=
 =?utf-8?B?T0szUUxiOGFNZG1kbjVoNzBOR2VmRFl5WVNySE1YenJCY2dQODhmUjBYc2Fn?=
 =?utf-8?B?ZmVHcUpUSDUxMjc0T3JUeDk1QS9OOEtObi9XMHd1UVkvQzZkSmRhaVJRb3da?=
 =?utf-8?B?S1VvK3VxZHNWT3hZdWJMbnZab2ZITjRjOGxHZVV3ZUlLOGJWRXJUbyswK3Ry?=
 =?utf-8?B?TzRqcjZ3SzY5emFoRWVwYjFJQ096cEJWMXUyc1Q5RHhyd0I1YVJJNHJTWTI3?=
 =?utf-8?B?ZGRtZ3VtNW9wUWdyalpEQ2lneGVnUUVYWHY2ZGIrazRjQkx0RjlyWU9BTjZw?=
 =?utf-8?B?KzVjcmxCdXNpekRHMFZPb2tuODZpbVNXdFNwbFNBTHpaM2RXeVBDckxyMm9u?=
 =?utf-8?B?MXhHUlhKVTBIbmsrcXJLSUtySjJLczFGMjdiYm1IS09uN1J4OTJMOU9xMmJJ?=
 =?utf-8?B?YW9VNmU5OUkwaGtPZzZObXYwYUhVSEQxRHBkR1AxS1podHhTVUxON3kvV2xP?=
 =?utf-8?B?MDBmakNTRTNNQ1RleEQ5WWNtdWpaS0VObnI1TDBmbTlsay8rNS9RWkEzcFBo?=
 =?utf-8?B?eWlrZHk1VUg1akllUnJlWnI4R25jNVdQOFh2ZmtCR3NIYUFhWXdCeVg2QlE3?=
 =?utf-8?B?VC83NHl4MDVmR3ViMU9zQTBJRElhQUQydjh4T0V4dVc5NVpWUlZhVTlGcGNt?=
 =?utf-8?B?dWpDUkNPTzludVNRaWsrbjhkQ3pyYUJGanhNdzI3MVNaN0ZwWjFiVG9ydjRv?=
 =?utf-8?B?QUJRckNteEhSRU5UZ29HUjFHbDNBRE9yMERlY242TURCZlpIZENSSFIxYVVQ?=
 =?utf-8?B?bURwOUtCQ0FqbEl3d3c4a1hoRHBWOEdhWm5VUUJmSEFTb3g1SnB2bzVDTStv?=
 =?utf-8?B?anZGZE1Fa1NBM3VvRlFySW1TVmNtdWx2eS9xK1FnRXp4N0NRYUhJSzZGS29o?=
 =?utf-8?B?VkdjL3M1L1ZIMmpwSmVROTg2YU5QRW5PcXhxU3huSjBNdm1CWDRyREM4Z0s5?=
 =?utf-8?B?WkFCeVdkRzE5RDNUSnlHWVVyenRLWVFQT0RSL2txeFFVYkNmVTdrWXphRXFj?=
 =?utf-8?B?QTF6OWlpZlZXZ3ZlK0dORzlxQUtiUjUxbllhSmgwem9tV3JSUHNKZXl0bWVX?=
 =?utf-8?B?ZFBzZG5kU3FtbjgzamIxTzlyMm1WeXUxeEJnSy9wSTgyTEhsSHJBQWtnM29B?=
 =?utf-8?B?K3c2UU1GZ3ZlbERKWnplc2I2VG00ZndSSEJ5N0xRUjR2Q3paWVc2R0IvOVZV?=
 =?utf-8?B?UTk0OEFTNkx6S2FBUmhGWTAwdDF4RGR5V3BoNHI2VDBmLzJ4VGlLT2dpZlpC?=
 =?utf-8?B?T3NwUGpsQzRTb0F6Y3hQbVAxbkVzeDE5YkhNM3JranRBaHJlLzFSRGZBSGJB?=
 =?utf-8?B?K0tod3ZRdzhUYXphRGJaODB0dTNyQzBZaUZkeTVGbS9nWW9ucVFrVC9uNU9w?=
 =?utf-8?B?SlF2RkxrQ0NzSG8xa1E4eWtwN05NWW9SNHJobmNnczVhSnI4WStNd2YvU2ZY?=
 =?utf-8?B?b1hlYW1EMkFuRnVISzBiNDBDU0ZjVlQ4YmFGbmYrVi9YWEYxYWdWQ2NmcXlx?=
 =?utf-8?B?Ukp3ODRISHRkQ1pqZVA5NEZTVWZlUEtkSTZsVFdsOVpzNDhUaEluMVNDTnNj?=
 =?utf-8?B?Z0dlbHAyNkFneGV5K1pGMFBQUGk0Vk5KZElVVEphV0JjQjhmZGUzc3BNK0FE?=
 =?utf-8?B?MFErbWJmbGlUc3pQT01ySmdPRHNrUHFIZnZwM1l1dHJsWjNLa0JHVHFrMXQz?=
 =?utf-8?B?Q2dlZFh1QjAzZFlIckZDZG9GTUdiakNIL1YwVFF4TkF5b203SlllbUZHcHFI?=
 =?utf-8?B?cUFOb0dNblBUdExuSGlVWXNDemFhbUo0SGU1VkRqRkJSd01QNWJEekF4NXpE?=
 =?utf-8?B?Uk5UYS8yWmZhTm5jS0hQMUtvN0lqWnF5eFhibzhRVlFEWGJjcFNWT1J0YW1o?=
 =?utf-8?B?RGJFUHMrM2RnVEY2dnJlOXF6cm82amVJbUI0SkIzTmc3bEROSTN4KzF4NXlS?=
 =?utf-8?B?SFhGOUZjb3FnLzgwc21mZGV2VTNnQ1krcnpmUmQ3Q3BHOWQ3SnpHdW1hdlpX?=
 =?utf-8?B?QzUySG8zSElPanlsSGlDZ2VXcWtyZzVmUnVkZWZHZEhDRyt0YlhGRndZWjFp?=
 =?utf-8?B?VitwYjA1RGxrazhoSU1DNmlYSkVQSmFSYWJDdVZ4eFBLNmYrd0dlMTRvV2Jz?=
 =?utf-8?B?d3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 58408683-9a5c-414b-fdae-08dbb52c8348
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Sep 2023 14:11:39.1980
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: MWTzi+Iv5EO9LzRfQPqazMn2o7iPhGHqhx1rU9PPYAiAOate1czzG3KQXZX9fkerAfqwDK2AkRk2e5mbANenijuymAhoyUA5GvZIv5P5Ss8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6735
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Thu, 14 Sep 2023 07:13:02 -0700 (PDT)
X-Spam-Status: No, score=-2.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email

From: Yajun Deng <yajun.deng@linux.dev>
Date: Thu, 14 Sep 2023 10:44:14 +0800

> 
> On 2023/9/13 17:58, Alexander Lobakin wrote:
>> From: Yajun Deng <yajun.deng@linux.dev>
>> Date: Wed, 13 Sep 2023 10:08:08 +0800
>>
>>> On 2023/9/13 00:22, Alexander Lobakin wrote:
>>>> From: Yajun Deng <yajun.deng@linux.dev>
>>>> Date: Mon, 11 Sep 2023 16:20:16 +0800
>> [...]
>>
>>>> EXPORT_SYMBOL_GPL(dev_core_stats_inc); // Why not GPL BTW?
>>> This may be a better option.
>>>
>>> Just because EXPORT_SYMBOL(netdev_core_stats_alloc) before,  but I think
>>>
>>> EXPORT_SYMBOL_GPL is better.
>> Ah I see. BTW, if you will still define increment functions as
>> externals, there will be no reason to export netdev_core_stats_alloc()
>> or even make it non-static at all.
>>
>>>  
>>>> And then build inlines:
>>>>
>>>> #define DEV_CORE_STATS_INC(FIELD)                \
>>>> static inline void                        \
>>>> dev_core_stats_##FIELD##_inc(struct net_device *dev)        \
>>>> {                                \
>>>>     dev_core_stats_inc(dev,                    \
>>>>         offsetof(struct net_device_core_stats, FIELD));    \
>>>> }
>>>>
>>>> DEV_CORE_STATS_INC(rx_dropped);
>>>> ...
>>>>
>>>> OR even just make them macros
>>>>
>>>> #define __DEV_CORE_STATS_INC(dev, field)            \
>>>>     dev_core_stats_inc(dev,                    \
>>>>         offsetof(struct net_device_core_stats, field))
>>>>
>>>> #define dev_core_stats_rx_dropped_inc(dev)            \
>>>>     __DEV_CORE_STATS_INC(dev, rx_dropped)
>>>> ...
>>> I would like the former.  Keep it the same as before.
>> By "the former" you mean to build static inlines or externals? Seems
>> like the first one, but I got confused by your "the same as before" :D
>>
>>>
>>>> Just don't copy that awful Thunderbird's line wrap and don't assume
>>>> this
>>>> code builds and works and that is something finished/polished.
>>>>
>>>> You'll be able to trace functions and you'll be able to understand
>>>> which
>>>> counter has been incremented by checking the second argument, i.e. the
>>>> field offset (IIRC tracing shows you arguments).
>>>> And that way you wouldn't geometrically increase the number of symbol
>>>> exports and deal with its consequences.
>>> I agree that.
>> Ok, after this one I guess you meant "I'd like to use your approach with
>> static inlines".
> 
> Finally, I give up this approach.
> 
> The new function dev_core_stats_inc() didn't called by external modules
> directly.

If it's called via an inline or macro or whatever, it still needs to be
exported.
Double-check that modpost doesn't complain on allmodconfig build.

> 
> So EXPORT_SYMBOL_GPL(dev_core_stats_inc) can be removed by anyone.

That doesn't mean it won't be needed tomorrow.
And I don't feel like it's a good excuse to define 1 external function
per counter instead of 1 external + static inlines for the rest. It's
not only about the exports.
Esp. given that I wrote almost the whole code needed for it to work in
one of my previous replies.

If you don't want to do that, I could take it over xD

> 
> 
>>>>>     /**
>>>>>    *    dev_get_stats    - get network device statistics
>>>> Thanks,
>>>> Olek
>> Thanks,
>> Olek

Thanks,
Olek