Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <9a1de9b3-b3cf-d26b-388e-c98294580bca@intel.com>
Date:   Tue, 12 Sep 2023 18:22:09 +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>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <20230911082016.3694700-1-yajun.deng@linux.dev>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bWZyVzBTaE0zRzRMU095b3lsZVRrU084YkJiUUlCa0JQNlNadDZLZW40L3U2?=
 =?utf-8?B?UjV0aXNjc2ZxOWQxbEZIM0FQZmYyZSszNHBMVzM0Q29JUS9TMDdoWCtTb1J0?=
 =?utf-8?B?VnFyVXNWcnprZFgza25rT2xMNitJVkIwa3o5bWVJeFVJaTVjNE1Nb24zam9r?=
 =?utf-8?B?a0I0MXZsTHVNNEVud3dNbjVRZWRWMU54bTBPalIyYTJsa1FCM2RRUFdzSXhM?=
 =?utf-8?B?RmpGZnB5MWtrZTVjRUZSYU83WHdmMi9UV3NCV2pTVWZtUUtDa1BBdjUzY25p?=
 =?utf-8?B?NXpZZGdrYzdkN2xOREFTaFZuOGhlYTE2UDJrQ3BCM2hQVS9FNzhCL1hrSUVS?=
 =?utf-8?B?azhiK1pJT1Njd3NjWG5qVzhxUytFekFYUStzS1B3cnFrRWZ1VmZwWjZ2anRS?=
 =?utf-8?B?MTdEWXZ5NDl0SGcvSm8ydE5sWVQyTGVrUElIcGRNVlFqcWoyM3o1aHRRYnM2?=
 =?utf-8?B?TkhIUUtmcmRBTFRLK0wwU0RpSm5SVFhCMTNVYi9SaFJ6VGdOWGlMbW96eFpl?=
 =?utf-8?B?Z24zeWJhWU13YUtFRDJBVEhtUHIvUUJmVHowR215dVR2cGFKZ2I5Y1B5NjZT?=
 =?utf-8?B?WFp5V1lkSnRndE5rUkNDQkN3K01oOWVIM0JHSGF3RGJYeXhLZ2ZrdTBydzhw?=
 =?utf-8?B?aHBybkhwNGtVR1dhQ2V1NEN0MGNLUUhPTnNJOUFqeGUraDFtd3k2dDVCbzRX?=
 =?utf-8?B?RjIweXZ3aXRDNTdMaGViTjc3UzJMSjF1dzY3OVIvbFRycFZ3V2lUN3o2U2Jy?=
 =?utf-8?B?eXVGcnQvcnZUMUpTbWc0SVh2cTZrVzhxSEtTSEQycm9HUmMwMVVtNTFTUHJs?=
 =?utf-8?B?TnBIcTUzcnFIam8va1IrUk16Q2h1RllrL3Btck1yYVplNldwZXhualFmRzRt?=
 =?utf-8?B?UjFkcEZSNGJ4Q2pDTCtLUlUxeDcwMkIrbnJvSGRDTnd3TGlKY1JCVVFVMTJY?=
 =?utf-8?B?QVZuR0x6bmllNkozWCsrc04zM3JqTDJsRW8wY2NHakxyL0tnaWNiR0FLS0VU?=
 =?utf-8?B?bE5DRG9rRGZCbzZhRmhCVllSZEhxUFNoWExVOUZzb252czZoQktoZ0I5emJ6?=
 =?utf-8?B?NFlreFhQeXFucWRnNndWSTVHTU5xMGZpZzdoZ2FzOVQ5dDZHUE1YZVpqQ0RT?=
 =?utf-8?B?TjBqT3JsUHoyK2VuSE9lTGh2dVRlaXVQeGw3OE5XY05NNTRMY3pVSHJvUUpE?=
 =?utf-8?B?NzJ6Zm4vSFpCcDA0dTdhWVJaTExFblVBdXgrclY1T2t5QUN5NDF3TUVvb0hv?=
 =?utf-8?B?TEE5NloxSWlPemo4R3ZkcDkzYk9yZlg2YjhWRk9tZllBNVB1Q1I5eG5rNnpo?=
 =?utf-8?B?enNhNDVEZnhBckprT2V1cWE3SHprV2Y1cXQ0cWN1NjdRRmN1aW5WT3RSdVZN?=
 =?utf-8?B?TitHQzJYdG1pWHhhUDlZTFcwMW0zcWZVK2RRSlVJMlh6V1dhVU9FWTZOWis4?=
 =?utf-8?B?QmZEdkl0bDdJS3lteG1vSUVsMkFwaWxiaUt2SCs1TFhOa3NodWp3MlB2YWhz?=
 =?utf-8?B?VlNmNFpzWDVrcHkxeVo4YU1wZEx6SXhLT015Yi9DbTJUdFV1eVA1Tm1jcHor?=
 =?utf-8?B?Kzc3L1RXYURkZ09pd1BEOHk3YUk0TDgzam9ZVDdZMUkwdHVxck4yVnVvU0lW?=
 =?utf-8?B?TENCWGkzVGVyeXV5WVpzelJ5aEJDek4rN2lheUEzN3Q2eFlMeDRJV3pVK1V2?=
 =?utf-8?B?OHVRWFJJb2dEZDhOdnlhWjJyN3UzZnNmVkp1RHQ3QmNidHIrWnlzczB0WXRq?=
 =?utf-8?B?Y25kcjBGc2IyaEdVdXRSeHBLbldKTFd3SzZZUkhjU0pJNmhxRExsOWVFVHJZ?=
 =?utf-8?B?V2VTcXBOV2dnc2FCNEZCZXlGU2gxbmltQ2c1Wk5ERFg3Q0xjOE9QOHFoOXps?=
 =?utf-8?B?M0JqTlY0L1RieUtkajFFV0RjVWgzRitvdHAvVnk4ekEvbkgzR0NvWTYvSkZz?=
 =?utf-8?B?bUVkL2Q2RGpZOHprNXptWVdrVk92SUxTWXRLOUY4bkVCT2hXY3VNOU92RUpu?=
 =?utf-8?B?ZVkxem9kUE9kcDEwdFFNdE43UGdvYUFObGpDQlJrcGNuSC9lMm82ZXc3em9M?=
 =?utf-8?B?Q0J2Y0Nhb2tIUTZjVzJuanRlN1RMZHduNXI3SkhqMnZSWUt6cDFoMUJxMXVS?=
 =?utf-8?B?Y1VoVi9hc3NKcTJhbktDWDNZZDl3UDVQdURwWU11VkdGT0dQZ0RYNVZ4NDF3?=
 =?utf-8?B?RUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f620beb1-3356-4303-3d5e-08dbb3ac7ebb
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Sep 2023 16:22:44.7082
 (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: TlZfC51Uc4kHjX/A4DJrYhr0V9LgPEPjMbdgVeFp9w7N9YhzNqbb2cHnhtNCtbp58+DDa7uGQ/ELVT8j2ioivRde9cYRXPvN8xnlioRqfgo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7365
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 (lipwig.vger.email [0.0.0.0]); Tue, 12 Sep 2023 09:31:04 -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 lipwig.vger.email

From: Yajun Deng <yajun.deng@linux.dev>
Date: Mon, 11 Sep 2023 16:20:16 +0800

> Although there is a kfree_skb_reason() helper function that can be used
> to find the reason for dropped packets, but most callers didn't increase
> one of rx_dropped, tx_dropped, rx_nohandler and rx_otherhost_dropped.

[...]

> diff --git a/net/core/dev.c b/net/core/dev.c
> index ccff2b6ef958..32ba730405b4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10475,7 +10475,7 @@ void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
>  }
>  EXPORT_SYMBOL(netdev_stats_to_stats64);
>  
> -struct net_device_core_stats __percpu *netdev_core_stats_alloc(struct net_device *dev)
> +static struct net_device_core_stats __percpu *netdev_core_stats_alloc(struct net_device *dev)
>  {
>  	struct net_device_core_stats __percpu *p;
>  
> @@ -10488,7 +10488,33 @@ struct net_device_core_stats __percpu *netdev_core_stats_alloc(struct net_device
>  	/* This READ_ONCE() pairs with the cmpxchg() above */
>  	return READ_ONCE(dev->core_stats);
>  }
> -EXPORT_SYMBOL(netdev_core_stats_alloc);
> +
> +static inline struct net_device_core_stats __percpu *dev_core_stats(struct net_device *dev)
> +{
> +	/* This READ_ONCE() pairs with the write in netdev_core_stats_alloc() */
> +	struct net_device_core_stats __percpu *p = READ_ONCE(dev->core_stats);
> +
> +	if (likely(p))
> +		return p;
> +
> +	return netdev_core_stats_alloc(dev);
> +}
> +
> +#define DEV_CORE_STATS_INC(FIELD)				\
> +void dev_core_stats_##FIELD##_inc(struct net_device *dev)	\
> +{								\
> +	struct net_device_core_stats __percpu *p;		\
> +								\
> +	p = dev_core_stats(dev);				\
> +	if (p)							\
> +		this_cpu_inc(p->FIELD);				\
> +}								\
> +EXPORT_SYMBOL(dev_core_stats_##FIELD##_inc)
> +
> +DEV_CORE_STATS_INC(rx_dropped);
> +DEV_CORE_STATS_INC(tx_dropped);
> +DEV_CORE_STATS_INC(rx_nohandler);
> +DEV_CORE_STATS_INC(rx_otherhost_dropped);

I realize you need to have an external function to be able to trace it,
but why don't you make it just 1 function instead of 4+ (will only be
increasing)?

Define 1 function

void dev_core_stats_inc(struct net_device *dev, u32 offset)
{
	struct net_device_core_stats __percpu *p;

	p = dev_core_stats(dev);
	if (p)
		this_cpu_inc(*(unsigned long *)(void *)p + offset);
}
EXPORT_SYMBOL_GPL(dev_core_stats_inc); // Why not GPL BTW?

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

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.

>  
>  /**
>   *	dev_get_stats	- get network device statistics

Thanks,
Olek