From: Eric Dumazet <eric.dumazet@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>,
"Jesper Dangaard Brouer" <brouer@redhat.com>
Cc: netdev@vger.kernel.org, bpf@vger.kernel.org,
"Daniel Borkmann" <daniel@iogearbox.net>,
"Alexei Starovoitov" <ast@kernel.org>,
"David Miller" <davem@davemloft.net>,
"Björn Töpel" <bjorn.topel@gmail.com>,
"John Fastabend" <john.fastabend@gmail.com>
Subject: Re: [PATCH bpf-next 1/2] xdp: Move devmap bulk queue into struct net_device
Date: Fri, 10 Jan 2020 14:46:35 -0800 [thread overview]
Message-ID: <968491aa-01c7-ae8d-4e7f-8ec58f1750b1@gmail.com> (raw)
In-Reply-To: <871rs7x7nc.fsf@toke.dk>
On 1/10/20 2:34 PM, Toke Høiland-Jørgensen wrote:
> Jesper Dangaard Brouer <brouer@redhat.com> writes:
>
>> On Fri, 10 Jan 2020 15:22:02 +0100
>> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index 2741aa35bec6..1b2bc2a7522e 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>> [...]
>>> @@ -1993,6 +1994,8 @@ struct net_device {
>>> spinlock_t tx_global_lock;
>>> int watchdog_timeo;
>>>
>>> + struct xdp_dev_bulk_queue __percpu *xdp_bulkq;
>>> +
>>> #ifdef CONFIG_XPS
>>> struct xps_dev_maps __rcu *xps_cpus_map;
>>> struct xps_dev_maps __rcu *xps_rxqs_map;
>>
>> We need to check that the cache-line for this location in struct
>> net_device is not getting updated (write operation) from different CPUs.
>>
>> The test you ran was a single queue single CPU test, which will not
>> show any regression for that case.
>
> Well, pahole says:
>
> /* --- cacheline 14 boundary (896 bytes) --- */
> struct netdev_queue * _tx __attribute__((__aligned__(64))); /* 896 8 */
> unsigned int num_tx_queues; /* 904 4 */
> unsigned int real_num_tx_queues; /* 908 4 */
> struct Qdisc * qdisc; /* 912 8 */
> struct hlist_head qdisc_hash[16]; /* 920 128 */
> /* --- cacheline 16 boundary (1024 bytes) was 24 bytes ago --- */
> unsigned int tx_queue_len; /* 1048 4 */
> spinlock_t tx_global_lock; /* 1052 4 */
> int watchdog_timeo; /* 1056 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> struct xdp_dev_bulk_queue * xdp_bulkq; /* 1064 8 */
> struct xps_dev_maps * xps_cpus_map; /* 1072 8 */
> struct xps_dev_maps * xps_rxqs_map; /* 1080 8 */
> /* --- cacheline 17 boundary (1088 bytes) --- */
>
>
> of those, tx_queue_len is the max queue len (so only set on init),
> tx_global_lock is not used by multi-queue devices, watchdog_timeo also
> seems to be a static value thats set on init, and the xps* pointers also
> only seems to be set once on init. So I think we're fine?
>
> I can run a multi-CPU test just to be sure, but I really don't see which
> of those fields might be updated on TX...
>
Note that another interesting field is miniq_egress, your patch
moves it to another cache line.
We probably should move qdisc_hash array elsewhere.
next prev parent reply other threads:[~2020-01-10 22:46 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-10 14:22 [PATCH bpf-next 0/2] xdp: Introduce bulking for non-map XDP_REDIRECT Toke Høiland-Jørgensen
2020-01-10 14:22 ` [PATCH bpf-next 1/2] xdp: Move devmap bulk queue into struct net_device Toke Høiland-Jørgensen
2020-01-10 15:03 ` Björn Töpel
2020-01-10 15:26 ` Toke Høiland-Jørgensen
2020-01-10 16:08 ` Jesper Dangaard Brouer
2020-01-10 22:34 ` Toke Høiland-Jørgensen
2020-01-10 22:46 ` Eric Dumazet [this message]
2020-01-10 23:16 ` Toke Høiland-Jørgensen
2020-01-10 14:22 ` [PATCH bpf-next 2/2] xdp: Use bulking for non-map XDP_REDIRECT Toke Høiland-Jørgensen
2020-01-10 15:15 ` Björn Töpel
2020-01-10 15:30 ` Toke Høiland-Jørgensen
2020-01-10 15:54 ` Björn Töpel
2020-01-10 15:57 ` Toke Høiland-Jørgensen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=968491aa-01c7-ae8d-4e7f-8ec58f1750b1@gmail.com \
--to=eric.dumazet@gmail.com \
--cc=ast@kernel.org \
--cc=bjorn.topel@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=brouer@redhat.com \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=john.fastabend@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=toke@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.