Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20230911082016.3694700-1-yajun.deng@linux.dev>
 <CANn89i+W1iAQmOhunLbqpvHu8EUO6uawv6Uvx7qimyBa_PBNCg@mail.gmail.com> <f3e84a37-3218-0d52-e7ed-2d215fed58e3@intel.com>
In-Reply-To: <f3e84a37-3218-0d52-e7ed-2d215fed58e3@intel.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Tue, 12 Sep 2023 18:04:44 +0200
Message-ID: <CANn89i+AwmpjM-bNuYRS26v-GRrVoucewxgmkvv25PNM4VWPGA@mail.gmail.com>
Subject: Re: [PATCH] net/core: Export dev_core_stats_rx_dropped_inc sets
To:     Alexander Lobakin <aleksander.lobakin@intel.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Sep 12, 2023 at 5:58=E2=80=AFPM 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=E2=80=AFAM Yajun Deng <yajun.deng@linux.d=
ev> wrote:
> >>
> >> Although there is a kfree_skb_reason() helper function that can be use=
d
> >> to find the reason for dropped packets, but most callers didn't increa=
se
> >> 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(s=
truct 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.
Forcing an inline makes no sense, this would duplicate the code four times,
for absolutely no gain.

> >
> >> +{
> >> +       /* This READ_ONCE() pairs with the write in netdev_core_stats_=
alloc() */
> >> +       struct net_device_core_stats __percpu *p =3D READ_ONCE(dev->co=
re_stats);
> >> +
> >> +       if (likely(p))
> >> +               return p;
> >> +
> >> +       return netdev_core_stats_alloc(dev);
> >> +}
>
> [...]
>
> Thanks,
> Olek