linux-kernel - Re: [PATCH v4] ixgbe: let the xdpdrv work with more than 64 cpus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL+tcoCovfAQmN_c43ScmjpO9D54CKP5XFTpx6VQpwJVxZhAdg@mail.gmail.com>
Date:   Fri, 27 Aug 2021 01:03:16 +0800
From:   Jason Xing <kerneljasonxing@...il.com>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        David Miller <davem@...emloft.net>, kuba@...nel.org,
        ast@...nel.org, daniel@...earbox.net, hawk@...nel.org,
        john.fastabend@...il.com, andrii@...nel.org, kafai@...com,
        songliubraving@...com, yhs@...com, kpsingh@...nel.org,
        intel-wired-lan@...ts.osuosl.org, netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, bpf@...r.kernel.org,
        Jason Xing <xingwanli@...ishou.com>,
        Shujin Li <lishujin@...ishou.com>
Subject: Re: [PATCH v4] ixgbe: let the xdpdrv work with more than 64 cpus

On Fri, Aug 27, 2021 at 12:41 AM Jesse Brandeburg
<jesse.brandeburg@...el.com> wrote:
>
> On 8/26/2021 9:18 AM, Eric Dumazet wrote:
>
> >> +static inline int ixgbe_determine_xdp_q_idx(int cpu)
> >> +{
> >> +    if (static_key_enabled(&ixgbe_xdp_locking_key))
> >> +            return cpu % IXGBE_MAX_XDP_QS;
> >> +    else
> >> +            return cpu;
> >
> > Even if num_online_cpus() is 8, the returned cpu here could be
> >
> > 0, 32, 64, 96, 128, 161, 197, 224
> >
> > Are we sure this will still be ok ?
>
> I'm not sure about that one myself. Jason?
>
> >
> >> +}
> >> +
> >>  static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter *adapter)
> >>  {
> >>      switch (adapter->hw.mac.type) {
> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
> >> index 0218f6c..884bf99 100644
> >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
> >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
> >> @@ -299,7 +299,10 @@ static void ixgbe_cache_ring_register(struct ixgbe_adapter *adapter)
> >>
> >>  static int ixgbe_xdp_queues(struct ixgbe_adapter *adapter)
> >>  {
> >> -    return adapter->xdp_prog ? nr_cpu_ids : 0;
> >> +    int queues;
> >> +
> >> +    queues = min_t(int, IXGBE_MAX_XDP_QS, num_online_cpus());
> >
> > num_online_cpus() might change later...
>
> I saw that too, but I wonder if it doesn't matter to the driver. If a
> CPU goes offline or comes online after the driver loads, we will use
> this logic to try to pick an available TX queue. But this is a
> complicated thing that is easy to get wrong, is there a common example
> of how to get it right?
>

Honestly, I'm a little confused right now. @nr_cpu_ids is the fixed
number which means the total number of cpus the machine has.
I think, using @nr_cpu_ids is safe one way or the other regardless of
whether the cpu goes offline or not. What do you think?

> A possible problem I guess is that if the "static_key_enabled" check
> returned false in the past, we would need to update that if the number
> of CPUs changes, do we need a notifier?
>

Things get complicated. If the number decreases down to
@IXGBE_MAX_XDP_QS (which is 64), the notifier could be useful because
we wouldn't need to use the @tx_lock. I'm wondering if we really need
to implement one notifier for this kind of change?

> Also, now that I'm asking it, I dislike the global as it would apply to
> all ixgbe ports and each PF would increment and decrement it
> independently. Showing my ignorance here, but I haven't seen this
> utility in the kernel before in detail. Not sure if this is "OK" from
> multiple device (with the same driver / global namespace) perspective.
>