Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <39c906f6-910d-01c7-404a-8fe6a161ef2e@intel.com>
Date:   Tue, 12 Sep 2023 19:15:39 +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:     Eric Dumazet <edumazet@google.com>
CC:     Yajun Deng <yajun.deng@linux.dev>, <davem@davemloft.net>,
        <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>
 <CANn89i+W1iAQmOhunLbqpvHu8EUO6uawv6Uvx7qimyBa_PBNCg@mail.gmail.com>
 <f3e84a37-3218-0d52-e7ed-2d215fed58e3@intel.com>
 <CANn89i+AwmpjM-bNuYRS26v-GRrVoucewxgmkvv25PNM4VWPGA@mail.gmail.com>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <CANn89i+AwmpjM-bNuYRS26v-GRrVoucewxgmkvv25PNM4VWPGA@mail.gmail.com>
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?dXB0cENXM0M5YkJpZS9XWGJzelFVamY5NGpaeWNpbXpqcWNnb00rSUFjOUxk?=
 =?utf-8?B?aHhDblNFNkZvNzE4QU96eUNMUllDSlNTSzRqaDhGWDlYaVJhVlB3YXBtSzl0?=
 =?utf-8?B?eWJJQjFIbm9BTEVRaHVObUs5T0d1ZUtLMFZYWlNhQjFvcWgvaXFCN2g1VzNo?=
 =?utf-8?B?QXRJaTBmQThaK1ZocW80Yy9zcU4rK2RDZ3BGS080NmZlR3lWUTBvTWZnR1VX?=
 =?utf-8?B?L0ttRitiZWNaWDc2aVU0QUxsSjh4YlBUSVo5cjJRM2ErMmRsYzNIcUpjYnhV?=
 =?utf-8?B?T240ajFhMkJYZk5CUmErM2hoM1cwSDNEM2NOYVF6MGRkcW4vWHJ4WVNEcUk2?=
 =?utf-8?B?Zmgyamh2c0pVNnluMDBmMExKbmI1dTRhRUNudXJucUdxdFFjcm5ad3JIbGdq?=
 =?utf-8?B?R0svMzQyRWlWcHRXY2VVQ05MWnpoNzRHaXJRMkpGM2xoSm91OHVQR1ZrbGpZ?=
 =?utf-8?B?Z1RuYVRLaXFrTFI4TjFkYVhUdlFUMTFUd29mb3JFd3lyYUJtbFhJbDFyVDRK?=
 =?utf-8?B?Q3ZpeWJodzFsbzBEbk1wZkhhcm9PKzh1UVA3aUdVYzkzY3kwaENGeGEwZnJM?=
 =?utf-8?B?bHpEbHIweldySG5QeTJQVU9YbnFaUEpqTVFzM1MwK295dEIyVDRXYXBNdkhu?=
 =?utf-8?B?bG5oVmg4Rko2Z1N3SUtWcGJvOWZxcEJiT29FT2cweUFFMzdvVmVRL3VqMUgx?=
 =?utf-8?B?eFVqaC9LbVlJT1lhd0pIaWYyRFdmOUtXbW03OE9kMWlPV3ZBelpoc0t2S3pJ?=
 =?utf-8?B?VThtWkZ2R1NKMzI2MTlDaHp6NEZETGhYbkpMWVdlL3dBZlRUSGZEeXYrYVdk?=
 =?utf-8?B?c0Fpb3pFdWVhZ0QvVUdTclZDNElIbXhlUE4yZDdicWNZYjR6NnpZeTVuaE9n?=
 =?utf-8?B?Q29mUURJTDhyNDEwbDVMQUZFNmtmM2E4K2VSOS9NQ2NBMDBMcHFlOVFqWWN1?=
 =?utf-8?B?TlNSMG5jL1RvcmZsNWlkRnU1cXB0MkxES3ZCdXE4TWdjS01iU1NTRUE4cGlG?=
 =?utf-8?B?NGc3TnVCclQ1NHVlRVJNQXZsVzJTYnZtY3NNaG1GcnB3Z2lodVZzSHk1Umx2?=
 =?utf-8?B?U2p3d1dXVmIxc3hONmI0bmtwc1NDSzhZdlNJL1huRE8ycEY3dVpaVFlldEpm?=
 =?utf-8?B?a1VIQ2c5ZVRlbGZZWkp0VFJCUHNNb25FOWZaWFgva3A4UTBML09kWHNpOWQ5?=
 =?utf-8?B?dC9USzY4MFZmckdSSWh6cFlmWjVrU2wyd1VsdDVLRUlsN044U3lzSEVpYnpw?=
 =?utf-8?B?c1EvZlg1eUJmSkxjZXMvYnpISzVwaCtxRWZ1c21lWnRVaE9kMzliU3N0ektn?=
 =?utf-8?B?NDJTVEZ2cUhqTzg0bUVqM2c0NWRjeWR4UWwyYUtMSjB4ejJIcTVBeU9KZVdE?=
 =?utf-8?B?enZ3MWM2LzB0U2FXTlU4UGRaNHFhdmE1TUllaVZpUFRLRHN1Wk5xb0tFMUli?=
 =?utf-8?B?Uk1Gai9temZad2JFajArVzUxaW1pVHlwZVN6dENlbm0vM1BwalB5bXlBMTNl?=
 =?utf-8?B?anJpVEpqaEk3M2pRTWVNOGlzSDhoN3lSYVJvVnA3K05keHMxdlZGenZXMlZG?=
 =?utf-8?B?RnVQUDA4OG9HTTRvOXFmOUtEQjZ2QWpHNUkvM3RDRVZXaHdhQnhZYkZJektD?=
 =?utf-8?B?dlJjMFJFNzVlbFRLdDR6a3JUMjNscHkvV1FpNVNRV3Y2cUFlMXp1cU1aRDhS?=
 =?utf-8?B?dlVhS3gya0Z3cUk3S1hyb2FpTzNzMkhZZUR5alg3ekQ5TzdjSlZ1K3NacWZy?=
 =?utf-8?B?ZmZoWCtrQlJaQ0VSanc1eFc4djkvR2xmYjZvUmpPaGtXQmUxQjZqUWtQcHlP?=
 =?utf-8?B?cHNDRjduMjVnVGZwd0YyN0xFRnNlQkpjMmZVRlMrTHpJLzBpb3JzM1ltZnRq?=
 =?utf-8?B?WTkyWUxnclEzbUV6QytMRTA1VjhCTm9OL0FyWEk2TEtOc3pRYzA1SkcxWVZs?=
 =?utf-8?B?b3VFNzhCaGJPSTVTaVM2WFRoSEJHbEJ4RUdZUlBSZk9oMEVrUHpEQVg4Mjlo?=
 =?utf-8?B?c2FRb09rZTNvbUw2bTdHaTVON0JIR3NGV3F4Qk5yOG03TUhUa3BlYlBYTXBN?=
 =?utf-8?B?NDRSTlFUTVpzdFVSSWJILzNYMXpOV0xUZytSZHZQWm9PZjMzVTNuRTdzZjFq?=
 =?utf-8?B?U1pPV2dvNlFSNk5CQWFOL1hHRS9iYnh6dzVWb2loYmZMYmxKdHp4Q29GK1p4?=
 =?utf-8?B?OWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f7a3a12c-cb67-4625-720e-08dbb3b3f800
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Sep 2023 17:16:14.5367
 (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: bTTregOq4LMm1kliLU7CNXpec4nJdrktjxokdz77GMB3AbchEuNXCqTOTzCMnDdyzNzxUiX9xxkhywe7FTEJUziktk/olPZtLeHXzCmG/4g=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8513
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 (snail.vger.email [0.0.0.0]); Tue, 12 Sep 2023 10:16:31 -0700 (PDT)

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 12 Sep 2023 18:04:44 +0200

> On Tue, Sep 12, 2023 at 5:58 PM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> From: Eric Dumazet <edumazet@google.com>
>> Date: Tue, 12 Sep 2023 06:23:24 +0200
>>
>>> On Mon, Sep 11, 2023 at 10:20 AM Yajun Deng <yajun.deng@linux.dev> wrote:
>>>>
>>>> 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.
>>
>> [...]
>>
>>>>  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)
>>>
>>> Please remove this inline attritbute. Consider using __cold instead.
>>
>> __cold? O_o I thought the author's inlining it as it's a couple
>> locs/intstructions, while the compilers would most likely keep it
>> non-inlined as it's referenced 4 times. __cold will for sure keep it
>> standalone and place it in .text.cold, i.e. far away from the call sites.
>> I realize dev_core_stats_*() aren't called frequently, but why making
>> only one small helper cold rather than all of them then?
>>
> 
> This helper is used at least one time per netdevice lifetime.
> This is definitely cold.

But then each dev_stats_*_inc() (not cold) has to call it from a
completely different piece of .text far from their. I either don't
understand the idea or dunno. Why not make them cold as well then?

> Forcing an inline makes no sense, this would duplicate the code four times,
> for absolutely no gain.

I'd love to see bloat-o-meter numbers, I suspect we're talking about
20-30 bytes.

> 
>>>
>>>> +{
>>>> +       /* 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);
>>>> +}
>>
>> [...]
>>
>> Thanks,
>> Olek

Thanks,
Olek