* Re: [net-next PATCH v4 5/7] net: Enable Tx queue selection based on Rx queues
From: Willem de Bruijn @ 2018-06-27 10:47 UTC (permalink / raw)
To: Amritha Nambiar
Cc: Network Development, David Miller, Alexander Duyck,
Samudrala, Sridhar, Alexander Duyck, Eric Dumazet,
Hannes Frederic Sowa, Tom Herbert
In-Reply-To: <90d56882-26a9-0a6e-414b-02bd777c2854@intel.com>
> >> +static int get_xps_queue(struct net_device *dev, struct sk_buff *skb)
> >> {
> >> #ifdef CONFIG_XPS
> >> struct xps_dev_maps *dev_maps;
> >> - struct xps_map *map;
> >> + struct sock *sk = skb->sk;
> >> int queue_index = -1;
> >>
> >> if (!static_key_false(&xps_needed))
> >> return -1;
> >>
> >> rcu_read_lock();
> >> - dev_maps = rcu_dereference(dev->xps_cpus_map);
> >> + if (!static_key_false(&xps_rxqs_needed))
> >> + goto get_cpus_map;
> >> +
> >> + dev_maps = rcu_dereference(dev->xps_rxqs_map);
> >> if (dev_maps) {
> >> - unsigned int tci = skb->sender_cpu - 1;
> >> + int tci = sk_rx_queue_get(sk);
> >
> > What if the rx device differs from the tx device?
> >
> I think I have 3 options here:
> 1. Cache the ifindex in sock_common which will introduce a new
> additional field in sock_common.
> 2. Use dev_get_by_napi_id to get the device id. This could be expensive,
> if the rxqs_map is set, this will be done on every packet and involves
> walking through the hashlist for napi_id lookup.
The tx queue mapping is cached in the sk for connected sockets, but
indeed this would be expensive for many workloads.
> 3. Remove validating device id, similar to how it is in skb_tx_hash
> where rx_queue recorded is used and if not, fall through to flow hash
> calculation.
> What do you think is suitable here?
Alternatively, just accept the misprediction in this rare case. But do
make the caveat explicit in the documentation.
^ permalink raw reply
* Re: [RFC bpf-next 6/6] samples/bpf: Add meta data hash example to xdp_redirect_cpu
From: Jesper Dangaard Brouer @ 2018-06-27 10:59 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Alexei Starovoitov, Daniel Borkmann, neerav.parikh, pjwaskiewicz,
ttoukan.linux, Tariq Toukan, alexander.h.duyck,
peter.waskiewicz.jr, Opher Reviv, Rony Efraim, netdev,
Saeed Mahameed, brouer
In-Reply-To: <20180627024615.17856-7-saeedm@mellanox.com>
On Tue, 26 Jun 2018 19:46:15 -0700
Saeed Mahameed <saeedm@dev.mellanox.co.il> wrote:
> Add a new program (prog_num = 4) that will not parse packets and will
> use the meta data hash to spread/redirect traffic into different cpus.
You cannot "steal" prognum 4, as it is already used for
"xdp_prognum4_ddos_filter_pktgen". Please append your new prog as #5.
> For the new program we set on bpf_set_link_xdp_fd:
> xdp_flags |= XDP_FLAGS_META_HASH | XDP_FLAGS_META_VLAN;
>
> On mlx5 it will succeed since mlx5 already supports these flags.
>
> The new program will read the value of the hash from the data_meta
> pointer from the xdp_md and will use it to compute the destination cpu.
>
> Note: I didn't test this patch to show redirect works with the hash!
> I only used it to see that the hash and vlan values are set correctly
> by the driver and can be seen by the xdp program.
>
> * I faced some difficulties to read the hash value using the helper
> functions defined in the previous patches, but once i used the same logic
> with out these functions it worked ! Will have to figure this out later.
>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
> samples/bpf/xdp_redirect_cpu_kern.c | 67 +++++++++++++++++++++++++++++
> samples/bpf/xdp_redirect_cpu_user.c | 7 +++
> 2 files changed, 74 insertions(+)
>
> diff --git a/samples/bpf/xdp_redirect_cpu_kern.c b/samples/bpf/xdp_redirect_cpu_kern.c
> index 303e9e7161f3..d6b3f55f342a 100644
> --- a/samples/bpf/xdp_redirect_cpu_kern.c
> +++ b/samples/bpf/xdp_redirect_cpu_kern.c
> @@ -376,6 +376,73 @@ int xdp_prognum3_proto_separate(struct xdp_md *ctx)
> return bpf_redirect_map(&cpu_map, cpu_dest, 0);
> }
>
> +#if 0
> +xdp_md_info_arr mdi = {
> + [XDP_DATA_META_HASH] = {.offset = 0, .present = 1},
> + [XDP_DATA_META_VLAN] = {.offset = sizeof(struct xdp_md_hash), .present = 1},
> +};
> +#endif
Sorry, no global variables avail in the generated BPF byte-code.
> +SEC("xdp_cpu_map4_hash_separate")
> +int xdp_prognum4_hash_separate(struct xdp_md *ctx)
> +{
> + void *data_meta = (void *)(long)ctx->data_meta;
> + void *data_end = (void *)(long)ctx->data_end;
> + void *data = (void *)(long)ctx->data;
> + struct xdp_md_hash *hash;
> + struct xdp_md_vlan *vlan;
> + struct datarec *rec;
> + u32 cpu_dest = 0;
> + u32 cpu_idx = 0;
> + u32 *cpu_lookup;
> + u32 key = 0;
> +
> + /* Count RX packet in map */
> + rec = bpf_map_lookup_elem(&rx_cnt, &key);
> + if (!rec)
> + return XDP_ABORTED;
> + rec->processed++;
> +
> + /* for some reason this code fails to be verified */
> +#if 0
> + hash = xdp_data_meta_get_hash(mdi, data_meta);
This will not work, because it is not implemented as a proper
BPF-helper call.
First, you currently store the xdp_md_info_arr inside the driver, which
makes it hard for a helper to access this. For helper access, we could
store this in xdp_rxq_info.
Second, in your design it looks like you are introducing a helper per
possible item in xdp_md_info_arr. I think we can reduce this to a
single helper, that takes a XDP_DATA_META_xxx flag, and returns an
offset. (The helper could return a direct pointer, but I don't think
the verfier can handle that, as it need to "see" this is related to
data_meta pointer, and that we do the proper boundry checks.).
The BPF prog already have direct memory access to the data_meta area,
and all it really need is an offset. Sure, the XDP/bpf programmer
could just calculate these offsets as constants, and remember to load
the XDP prog with the flags that corresponds to the calculated offsets.
But I think we can do something even smarter...
It should be possible to convert/patch the BPF instructions, of the
helper call that returns an offset, to instead avoid the call and
either (1) provide the offset as a constant/IMM or (2) make BPF inst
doing the lookup in xdp_md_info_arr.
> + if (hash + 1 > data)
> + return XDP_ABORTED;
> +
> + vlan = xdp_data_meta_get_vlan(mdi, data_meta);
> + if (vlan + 1 > data)
> + return XDP_ABORTED;
> +#endif
> +
> + /* Work around for the above code */
> + hash = data_meta; /* since we know hash will appear first */
> + if (hash + 1 > data)
> + return XDP_ABORTED;
> +
> +#if 0
> + // Just for testing
> + /* We know that vlan will appear after the hash */
> + vlan = (void *)((char *)data_meta + sizeof(*hash));
> + if (vlan + 1 > data) {
> + return XDP_ABORTED;
> + }
> +#endif
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [bpf-next PATCH 2/2] samples/bpf: xdp_rxq_info action XDP_TX must adjust MAC-addrs
From: Jesper Dangaard Brouer @ 2018-06-27 11:20 UTC (permalink / raw)
To: Song Liu
Cc: Networking, Daniel Borkmann, Toke Høiland-Jørgensen,
Alexei Starovoitov, brouer
In-Reply-To: <CAPhsuW74ZzHawToa3YXxzT6ojk+ScdCvcO3KyT3EBcCxqPHDXg@mail.gmail.com>
On Tue, 26 Jun 2018 17:09:01 -0700
Song Liu <liu.song.a23@gmail.com> wrote:
> On Mon, Jun 25, 2018 at 7:27 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > XDP_TX requires also changing the MAC-addrs, else some hardware
> > may drop the TX packet before reaching the wire. This was
> > observed with driver mlx5.
> >
> > If xdp_rxq_info select --action XDP_TX the swapmac functionality
> > is activated. It is also possible to manually enable via cmdline
> > option --swapmac. This is practical if wanting to measure the
> > overhead of writing/updating payload for other action types.
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> > ---
[...]
> >
> > diff --git a/samples/bpf/xdp_rxq_info_kern.c b/samples/bpf/xdp_rxq_info_kern.c
> > index 61af6210df2f..222a83eed1cb 100644
> > --- a/samples/bpf/xdp_rxq_info_kern.c
> > +++ b/samples/bpf/xdp_rxq_info_kern.c
> > @@ -21,6 +21,7 @@ struct config {
> > enum cfg_options_flags {
> > NO_TOUCH = 0x0U,
> > READ_MEM = 0x1U,
> > + SWAP_MAC = 0x2U,
> > };
[...]
> > @@ -98,7 +116,7 @@ int xdp_prognum0(struct xdp_md *ctx)
> > rxq_rec->issue++;
> >
> > /* Default: Don't touch packet data, only count packets */
> > - if (unlikely(config->options & READ_MEM)) {
> > + if (unlikely(config->options & (READ_MEM|SWAP_MAC))) {
> > struct ethhdr *eth = data;
> >
> > if (eth + 1 > data_end)
[...]
> > diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
> > index 435485d4f49e..248a7eab9531 100644
[...]
> > @@ -119,6 +121,8 @@ static char* options2str(enum cfg_options_flags flag)
> > {
> > if (flag == NO_TOUCH)
> > return "no_touch";
> > + if (flag & SWAP_MAC)
> > + return "swapmac";
> > if (flag & READ_MEM)
> > return "read";
>
> I guess SWAP_MAC also reads the memory, so it "includes" READ_MEM?
True (see _kern side)
> It is OK for now. We may need to refactor this part when adding other
> flags in the future.
Sure, do remember that this is only a 'sample' program.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [bpf-next PATCH 1/2] samples/bpf: extend xdp_rxq_info to read packet payload
From: Jesper Dangaard Brouer @ 2018-06-27 11:23 UTC (permalink / raw)
To: Song Liu
Cc: Networking, Daniel Borkmann, Toke Høiland-Jørgensen,
Alexei Starovoitov, brouer
In-Reply-To: <CAPhsuW7i_YsrFvtrFwAybOiFFMUjQzQwtOwVz-r0=d9=TPM=Cw@mail.gmail.com>
On Tue, 26 Jun 2018 16:53:15 -0700
Song Liu <liu.song.a23@gmail.com> wrote:
> > +static char* options2str(enum cfg_options_flags flag)
> > +{
> > + if (flag == NO_TOUCH)
> > + return "no_touch";
> > + if (flag & READ_MEM)
> > + return "read";
> > + fprintf(stderr, "ERR: Unknown config option flags");
> > + exit(EXIT_FAIL);
> > +}
> > +
>
> enum cfg_options_flags is used as a bitmap in other parts of the sample.
> So this function is a little weird (with more flags added).
Sure, and do I handle this correctly in the next patch.
I'm uncertain what you want me to change?
Do you want me to drop the enum, and use #define instead?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
From: Okash Khawaja @ 2018-06-27 11:47 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jakub Kicinski, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
In-Reply-To: <23968d53-2895-f0bc-a38c-5bc99e1846ab@iogearbox.net>
On Wed, Jun 27, 2018 at 12:34:35PM +0200, Daniel Borkmann wrote:
> On 06/27/2018 12:35 AM, Jakub Kicinski wrote:
> > On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:
> >> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
> [...]
> >>> Implementing both outputs in one series will help you structure your
> >>> code to best suit both of the formats up front.
> >> hex and "formatted" are the only things missing? As always, things
> >> can be refactored when new use case comes up. Lets wait for
> >> Okash input.
> >>
> >> Regardless, plaintext is our current use case. Having the current
> >> patchset in does not stop us or others from contributing other use
> >> cases (json, "bpftool map find"...etc), and IMO it is actually
> >> the opposite. Others may help us get there faster than us alone.
> >> We should not stop making forward progress and take this patch
> >> as hostage because "abc" and "xyz" are not done together.
> >
> > Parity between JSON and plain text output is non negotiable.
>
> Longish discussion and some confusion in this thread. :-) First of all
> thanks a lot for working on it, very useful!
Thanks :)
> My $0.02 on it is that so far
> great care has been taken in bpftool to indeed have feature parity between
> JSON and plain text, so it would be highly desirable to keep continuing
> this practice if the consensus is that it indeed is feasible and makes
> sense wrt BTF data. There has been mentioned that given BTF data can be
> dynamic depending on what the user loads via bpf(2) so a potential JSON
> output may look different/break each time anyway. This however could all be
> embedded under a container object that has a fixed key like 'formatted'
> where tools like jq(1) can query into it. I think this would be fine since
> the rest of the (non-dynamic) output is still retained as-is and then
> wouldn't confuse or collide with existing users, and anyone programmatically
> parsing deeper into the BTF data under such JSON container object needs
> to have awareness of what specific data it wants to query from it; so
> there's no conflict wrt breaking anything here. Imho, both outputs would
> be very valuable.
Okay I can add "formatted" object under json output.
One thing to note here is that the fixed output will change if the map
itself changes. So someone writing a program that consumes that fixed
output will have to account for his program breaking in future, thus
breaking backward compatibility anyway as far as the developer is
concerned :)
I will go ahead with work on "formatted" object.
Thanks,
Okash
>
> Thanks,
> Daniel
^ permalink raw reply
* [PATCH net-next] tcp: replace LINUX_MIB_TCPOFODROP with LINUX_MIB_TCPRMEMFULLDROP for drops due to receive buffer full
From: Yafang Shao @ 2018-06-27 11:50 UTC (permalink / raw)
To: edumazet, davem; +Cc: netdev, linux-kernel, Yafang Shao
When sk_rmem_alloc is larger than the receive buffer and we can't
schedule more memory for it, the skb will be dropped.
In above situation, if this skb is put into the ofo queue,
LINUX_MIB_TCPOFODROP is incremented to track it,
while if this skb is put into the receive queue, there's no record.
So LINUX_MIB_TCPOFODROP is replaced with LINUX_MIB_TCPRMEMFULLDROP to track
this behavior.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
include/uapi/linux/snmp.h | 2 +-
net/ipv4/proc.c | 2 +-
net/ipv4/tcp_input.c | 10 +++++++---
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
index 97517f3..3a83322 100644
--- a/include/uapi/linux/snmp.h
+++ b/include/uapi/linux/snmp.h
@@ -243,7 +243,6 @@ enum
LINUX_MIB_TCPRETRANSFAIL, /* TCPRetransFail */
LINUX_MIB_TCPRCVCOALESCE, /* TCPRcvCoalesce */
LINUX_MIB_TCPOFOQUEUE, /* TCPOFOQueue */
- LINUX_MIB_TCPOFODROP, /* TCPOFODrop */
LINUX_MIB_TCPOFOMERGE, /* TCPOFOMerge */
LINUX_MIB_TCPCHALLENGEACK, /* TCPChallengeACK */
LINUX_MIB_TCPSYNCHALLENGE, /* TCPSYNChallenge */
@@ -280,6 +279,7 @@ enum
LINUX_MIB_TCPDELIVEREDCE, /* TCPDeliveredCE */
LINUX_MIB_TCPACKCOMPRESSED, /* TCPAckCompressed */
LINUX_MIB_TCPZEROWINDOWDROP, /* TCPZeroWindowDrop */
+ LINUX_MIB_TCPRMEMFULLDROP, /* TCPRmemFullDrop */
__LINUX_MIB_MAX
};
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 225ef34..43ee02d 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -251,7 +251,6 @@ static int sockstat_seq_show(struct seq_file *seq, void *v)
SNMP_MIB_ITEM("TCPRetransFail", LINUX_MIB_TCPRETRANSFAIL),
SNMP_MIB_ITEM("TCPRcvCoalesce", LINUX_MIB_TCPRCVCOALESCE),
SNMP_MIB_ITEM("TCPOFOQueue", LINUX_MIB_TCPOFOQUEUE),
- SNMP_MIB_ITEM("TCPOFODrop", LINUX_MIB_TCPOFODROP),
SNMP_MIB_ITEM("TCPOFOMerge", LINUX_MIB_TCPOFOMERGE),
SNMP_MIB_ITEM("TCPChallengeACK", LINUX_MIB_TCPCHALLENGEACK),
SNMP_MIB_ITEM("TCPSYNChallenge", LINUX_MIB_TCPSYNCHALLENGE),
@@ -288,6 +287,7 @@ static int sockstat_seq_show(struct seq_file *seq, void *v)
SNMP_MIB_ITEM("TCPDeliveredCE", LINUX_MIB_TCPDELIVEREDCE),
SNMP_MIB_ITEM("TCPAckCompressed", LINUX_MIB_TCPACKCOMPRESSED),
SNMP_MIB_ITEM("TCPZeroWindowDrop", LINUX_MIB_TCPZEROWINDOWDROP),
+ SNMP_MIB_ITEM("TCPRmemFullDrop", LINUX_MIB_TCPRMEMFULLDROP),
SNMP_MIB_SENTINEL
};
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 9c5b341..d11abb8 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4442,7 +4442,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
tcp_ecn_check_ce(sk, skb);
if (unlikely(tcp_try_rmem_schedule(sk, skb, skb->truesize))) {
- NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFODROP);
+ NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPRMEMFULLDROP);
tcp_drop(sk, skb);
return;
}
@@ -4611,8 +4611,10 @@ int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
skb->data_len = data_len;
skb->len = size;
- if (tcp_try_rmem_schedule(sk, skb, skb->truesize))
+ if (tcp_try_rmem_schedule(sk, skb, skb->truesize)) {
+ NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPRMEMFULLDROP);
goto err_free;
+ }
err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, size);
if (err)
@@ -4677,8 +4679,10 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
queue_and_out:
if (skb_queue_len(&sk->sk_receive_queue) == 0)
sk_forced_mem_schedule(sk, skb->truesize);
- else if (tcp_try_rmem_schedule(sk, skb, skb->truesize))
+ else if (tcp_try_rmem_schedule(sk, skb, skb->truesize)) {
+ NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPRMEMFULLDROP);
goto drop;
+ }
eaten = tcp_queue_rcv(sk, skb, 0, &fragstolen);
tcp_rcv_nxt_update(tp, TCP_SKB_CB(skb)->end_seq);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next] mlx4: do not use rwlock in fast path
From: Tariq Toukan @ 2018-06-27 12:11 UTC (permalink / raw)
To: Eric Dumazet, David Miller
Cc: netdev, Tariq Toukan, Shawn Bohrer, Shay Agroskin,
Eran Ben Elisha
In-Reply-To: <1486660204.7793.104.camel@edumazet-glaptop3.roam.corp.google.com>
On 09/02/2017 7:10 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Using a reader-writer lock in fast path is silly, when we can
> instead use RCU or a seqlock.
>
> For mlx4 hwstamp clock, a seqlock is the way to go, removing
> two atomic operations and false sharing.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Tariq Toukan <tariqt@mellanox.com>
> ---
> drivers/net/ethernet/mellanox/mlx4/en_clock.c | 35 ++++++++--------
> drivers/net/ethernet/mellanox/mlx4/mlx4_en.h | 2
> 2 files changed, 19 insertions(+), 18 deletions(-)
>
Hi Eric,
When my peer, Shay, modified mlx5 to adopt this same locking
scheme/type, he noticed a degradation in packet rate.
He got back to testing mlx4 and also noticed a degradation introduced by
this patch.
Perf numbers (single ring):
mlx4:
with rw-lock: ~8.54M pps
with seq-lock: ~8.51M pps
mlx5:
With rw-lock: ~14.94M pps
With seq-lock: ~14.48M pps
Actually, this can be explained by the analysis below.
In short, number of readers is significantly larger than of writers.
Hence optimizing the readers flow would give better numbers. The issue
is, the read/write lock might cause writers starvation. Maybe RCU fits
best here?
Degradation analysis:
The patch changes the lock type which protects reads and updates of a
variable ( (struct mlx4_en_dev).clock variable)
This variable is used to convert the hw timestamp into skb->hwtstamps.
This variable is read for each transmitted/received packet and updated
only via ptp module and some overflow periodic work we have (maximum of
10 times per second)
Meaning that there are much more readers than writers, and it’s best to
optimize the readers flow.
Best,
Tariq
^ permalink raw reply
* Re: s390x BPF JIT failures with test_bpf
From: Kleber Souza @ 2018-06-27 12:30 UTC (permalink / raw)
To: Daniel Borkmann, linux-s390, netdev; +Cc: Alexei Starovoitov
In-Reply-To: <0511ac38-beec-131f-5643-04eee3357cc4@iogearbox.net>
On 06/27/18 12:36, Daniel Borkmann wrote:
> On 06/27/2018 12:13 PM, Kleber Souza wrote:
>> On 06/27/18 12:01, Daniel Borkmann wrote:
>>> On 06/27/2018 11:40 AM, Kleber Souza wrote:
>>> [...]
>>>> When I load the test_bpf module from mainline (v4.18-rc2) with
>>>> CONFIG_BPF_JIT_ALWAYS_ON=y on a s390x system I get the following errors:
>>>>
>>>> test_bpf: #289 BPF_MAXINSNS: Ctx heavy transformations FAIL to
>>>> prog_create err=-524 len=4096
>>>> test_bpf: #290 BPF_MAXINSNS: Call heavy transformations FAIL to
>>>> prog_create err=-524 len=4096
>>>> [...]
>>>> test_bpf: #296 BPF_MAXINSNS: exec all MSH FAIL to prog_create err=-524
>>>> len=4096
>>>> test_bpf: #297 BPF_MAXINSNS: ld_abs+get_processor_id FAIL to prog_create
>>>> err=-524 len=4096
>>>>
>>>> From a quick look at the code it seems that
>>>> arch/s390/net/bpf_jit_comp.c:bpf_int_jit_compile() is failing to JIT
>>>> compile the test code.
>>>>
>>>> Are those failures expected and could be flagged with FLAG_EXPECTED_FAIL
>>>> on lib/test_bpf.c or are those caused by some issue with the s390x JIT
>>>> compiler that needs to be fixed?
>>>
>>> JIT doesn't guarantee in general to map really all programs to native insns,
>>> so some, mostly crafted corner cases could fail. E.g. x86-64 JIT doesn't converge
>>> on some programs in test_bpf.c and thus falls back to interpreter or simply
>>> rejects the program in case of CONFIG_BPF_JIT_ALWAYS_ON=y. Above would seem
>>> likely that it's hitting the BPF_SIZE_MAX that s390 would do. I think it might
>>> make sense to either have the FLAG_EXPECTED_FAIL in lib/test_bpf.c more fine
>>> grained as a flag per arch, so we could say it's expected to fail on e.g. s390
>>> but not on x86 and the like, or just denote it as 'could potentially fail but
>>> doesn't have to be the case everywhere'.
>>
>> Thank you for your reply. I will run some more tests to make sure we are
>> hitting BPF_SIZE_MAX or what exactly is failing and send a patch to flag
>> it conditionally for s390x.
>
> Sounds good, thanks! In any case, please let us know your findings.
>
> Best,
> Daniel
>
Hi Daniel,
Your presumption was correct, all four tests are failing because they
exceed BPF_SIZE_MAX. I'll send a patch shortly.
Thanks!
Kleber
^ permalink raw reply
* Re: [PATCH net-next] net: preserve sock reference when scrubbing the skb.
From: Flavio Leitner @ 2018-06-27 12:31 UTC (permalink / raw)
To: Cong Wang
Cc: Eric Dumazet, Linux Kernel Network Developers, Paolo Abeni,
David Miller, Florian Westphal, NetFilter
In-Reply-To: <CAM_iQpWxHb8VrHUQAEgOQ0YsSVt5MMZvGvAQVuA-JGcfjc=ubg@mail.gmail.com>
On Tue, Jun 26, 2018 at 06:28:27PM -0700, Cong Wang wrote:
> On Tue, Jun 26, 2018 at 5:39 PM Flavio Leitner <fbl@redhat.com> wrote:
> >
> > On Tue, Jun 26, 2018 at 05:29:51PM -0700, Cong Wang wrote:
> > > On Tue, Jun 26, 2018 at 4:33 PM Flavio Leitner <fbl@redhat.com> wrote:
> > > >
> > > > It is still isolated, the sk carries the netns info and it is
> > > > orphaned when it re-enters the stack.
> > >
> > > Then what difference does your patch make?
> >
> > Don't forget it is fixing two issues.
>
> Sure. I am only talking about TSQ from the very beginning.
> Let me rephrase my above question:
> What difference does your patch make to TSQ?
It avoids burstiness.
> > > Before your patch:
> > > veth orphans skb in its xmit
> > >
> > > After your patch:
> > > RX orphans it when re-entering stack (as you claimed, I don't know)
> >
> > ip_rcv, and equivalents.
>
> ip_rcv() is L3, we enter a stack from L1. So your above claim is incorrect. :)
Maybe you found a problem, could you please point me to where in
between L1 to L3 the socket is relevant?
> > > And for veth pair:
> > > xmit from one side is RX for the other side
> > > So, where is the queueing? Where is the buffer bloat? GRO list??
> >
> > CPU backlog.
>
> Yeah, but this is never targeted by TSQ:
>
> tcp_limit_output_bytes limits the number of bytes on qdisc
> or device to reduce artificial RTT/cwnd and reduce bufferbloat.
>
> which means you have to update Documentation/networking/ip-sysctl.txt
> too.
How it is never targeted? Whole point is to avoid queueing traffic.
Would you be okay if I include this chunk?
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index ce8fbf5aa63c..f4c042be0216 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -733,11 +733,11 @@ tcp_limit_output_bytes - INTEGER
Controls TCP Small Queue limit per tcp socket.
TCP bulk sender tends to increase packets in flight until it
gets losses notifications. With SNDBUF autotuning, this can
- result in a large amount of packets queued in qdisc/device
- on the local machine, hurting latency of other flows, for
- typical pfifo_fast qdiscs.
- tcp_limit_output_bytes limits the number of bytes on qdisc
- or device to reduce artificial RTT/cwnd and reduce bufferbloat.
+ result in a large amount of packets queued on the local machine
+ (e.g.: qdiscs, CPU backlog, or device) hurting latency of other
+ flows, for typical pfifo_fast qdiscs. tcp_limit_output_bytes
+ limits the number of bytes on qdisc or device to reduce artificial
+ RTT/cwnd and reduce bufferbloat.
Default: 262144
tcp_challenge_ack_limit - INTEGER
--
Flavio
^ permalink raw reply related
* Re: [RFC PATCH v2 net-next 07/12] net: ipv4: listified version of ip_rcv
From: Florian Westphal @ 2018-06-27 12:32 UTC (permalink / raw)
To: Edward Cree; +Cc: linux-net-drivers, netdev, davem
In-Reply-To: <f4b330bf-a1b3-2849-4265-faa0aaa1e941@solarflare.com>
Edward Cree <ecree@solarflare.com> wrote:
> Also involved adding a way to run a netfilter hook over a list of packets.
> Rather than attempting to make netfilter know about lists (which would be
> a major project in itself) we just let it call the regular okfn (in this
> case ip_rcv_finish()) for any packets it steals, and have it give us back
> a list of packets it's synchronously accepted (which normally NF_HOOK
> would automatically call okfn() on, but we want to be able to potentially
> pass the list to a listified version of okfn().)
okfn() is only used during async reinject in NFQUEUE case,
skb is queued in kernel and we'll wait for a verdict from a userspace
process. If thats ACCEPT, then okfn() gets called to reinject the skb
into the network stack.
A normal -j ACCEPT doesn't call okfn in the netfilter core, which is why
this occurs on '1' retval in NF_HOOK().
Only other user of okfn() is bridge netfilter, so listified version of
okfn() doesn't make too much sense to me, its not used normally
(unless such listified version makes the code simpler of course).
AFAICS its fine to unlink/free skbs from the list to handle
drops/queueing etc. so a future version of nf_hook() could propagate the
list into nf_hook_slow and mangle the list there to deal with hooks
that steal/drop/queue skbs.
Later on we can pass the list to the hook functions themselves.
We'll have to handle non-accept verdicts in-place in the hook functions
for this, but fortunately most hookfns only return NF_ACCEPT so I think
it is manageable.
I'll look into this once the series makes it to net-next.
^ permalink raw reply
* [PATCH net-next] net: stmmac: Add support for CBS QDISC
From: Jose Abreu @ 2018-06-27 12:43 UTC (permalink / raw)
To: netdev
Cc: Jose Abreu, David S. Miller, Joao Pinto, Vitor Soares,
Giuseppe Cavallaro, Alexandre Torgue
This adds support for CBS reconfiguration using the TC application.
A new callback was added to TC ops struct and another one to DMA ops to
reconfigure the channel mode.
Tested in GMAC5.10.
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Vitor Soares <soares@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
---
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 15 ++++++
drivers/net/ethernet/stmicro/stmmac/hwif.h | 8 +++
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +
drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 62 +++++++++++++++++++++++
4 files changed, 87 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index d37f17ca62fe..6e32f8a3710b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -407,6 +407,19 @@ static void dwmac4_enable_tso(void __iomem *ioaddr, bool en, u32 chan)
}
}
+static void dwmac4_qmode(void __iomem *ioaddr, u32 channel, u8 qmode)
+{
+ u32 mtl_tx_op = readl(ioaddr + MTL_CHAN_TX_OP_MODE(channel));
+
+ mtl_tx_op &= ~MTL_OP_MODE_TXQEN_MASK;
+ if (qmode != MTL_QUEUE_AVB)
+ mtl_tx_op |= MTL_OP_MODE_TXQEN;
+ else
+ mtl_tx_op |= MTL_OP_MODE_TXQEN_AV;
+
+ writel(mtl_tx_op, ioaddr + MTL_CHAN_TX_OP_MODE(channel));
+}
+
const struct stmmac_dma_ops dwmac4_dma_ops = {
.reset = dwmac4_dma_reset,
.init = dwmac4_dma_init,
@@ -431,6 +444,7 @@ const struct stmmac_dma_ops dwmac4_dma_ops = {
.set_rx_tail_ptr = dwmac4_set_rx_tail_ptr,
.set_tx_tail_ptr = dwmac4_set_tx_tail_ptr,
.enable_tso = dwmac4_enable_tso,
+ .qmode = dwmac4_qmode,
};
const struct stmmac_dma_ops dwmac410_dma_ops = {
@@ -457,4 +471,5 @@ const struct stmmac_dma_ops dwmac410_dma_ops = {
.set_rx_tail_ptr = dwmac4_set_rx_tail_ptr,
.set_tx_tail_ptr = dwmac4_set_tx_tail_ptr,
.enable_tso = dwmac4_enable_tso,
+ .qmode = dwmac4_qmode,
};
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index e44e7b26ce82..e2a965790648 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -183,6 +183,7 @@ struct stmmac_dma_ops {
void (*set_rx_tail_ptr)(void __iomem *ioaddr, u32 tail_ptr, u32 chan);
void (*set_tx_tail_ptr)(void __iomem *ioaddr, u32 tail_ptr, u32 chan);
void (*enable_tso)(void __iomem *ioaddr, bool en, u32 chan);
+ void (*qmode)(void __iomem *ioaddr, u32 channel, u8 qmode);
};
#define stmmac_reset(__priv, __args...) \
@@ -235,6 +236,8 @@ struct stmmac_dma_ops {
stmmac_do_void_callback(__priv, dma, set_tx_tail_ptr, __args)
#define stmmac_enable_tso(__priv, __args...) \
stmmac_do_void_callback(__priv, dma, enable_tso, __args)
+#define stmmac_dma_qmode(__priv, __args...) \
+ stmmac_do_void_callback(__priv, dma, qmode, __args)
struct mac_device_info;
struct net_device;
@@ -441,17 +444,22 @@ struct stmmac_mode_ops {
struct stmmac_priv;
struct tc_cls_u32_offload;
+struct tc_cbs_qopt_offload;
struct stmmac_tc_ops {
int (*init)(struct stmmac_priv *priv);
int (*setup_cls_u32)(struct stmmac_priv *priv,
struct tc_cls_u32_offload *cls);
+ int (*setup_cbs)(struct stmmac_priv *priv,
+ struct tc_cbs_qopt_offload *qopt);
};
#define stmmac_tc_init(__priv, __args...) \
stmmac_do_callback(__priv, tc, init, __args)
#define stmmac_tc_setup_cls_u32(__priv, __args...) \
stmmac_do_callback(__priv, tc, setup_cls_u32, __args)
+#define stmmac_tc_setup_cbs(__priv, __args...) \
+ stmmac_do_callback(__priv, tc, setup_cbs, __args)
struct stmmac_regs_off {
u32 ptp_off;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ccf3e2e161ca..a6be3b59a728 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3778,6 +3778,8 @@ static int stmmac_setup_tc(struct net_device *ndev, enum tc_setup_type type,
switch (type) {
case TC_SETUP_BLOCK:
return stmmac_setup_tc_block(priv, type_data);
+ case TC_SETUP_QDISC_CBS:
+ return stmmac_tc_setup_cbs(priv, priv, type_data);
default:
return -EOPNOTSUPP;
}
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 881c94b73e2f..67542bd21010 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -289,7 +289,69 @@ static int tc_init(struct stmmac_priv *priv)
return 0;
}
+static int tc_setup_cbs(struct stmmac_priv *priv,
+ struct tc_cbs_qopt_offload *qopt)
+{
+ u32 tx_queues_count = priv->plat->tx_queues_to_use;
+ u32 queue = qopt->queue;
+ u32 ptr, speed_div;
+ u32 mode_to_use;
+ s64 value;
+ int ret;
+
+ /* Queue 0 is not AVB capable */
+ if (queue <= 0 || queue >= tx_queues_count)
+ return -EINVAL;
+ if (priv->speed != SPEED_100 && priv->speed != SPEED_1000)
+ return -EOPNOTSUPP;
+
+ mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
+ if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
+ ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_AVB);
+ if (ret)
+ return ret;
+
+ priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
+ } else if (!qopt->enable) {
+ return stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_DCB);
+ }
+
+ /* Port Transmit Rate and Speed Divider */
+ ptr = (priv->speed == SPEED_100) ? 4 : 8;
+ speed_div = (priv->speed == SPEED_100) ? 100000 : 1000000;
+
+ /* Final adjustments for HW */
+ value = qopt->idleslope * 1024 * ptr;
+ do_div(value, speed_div);
+ priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);
+
+ value = -qopt->sendslope * 1024UL * ptr;
+ do_div(value, speed_div);
+ priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0);
+
+ value = qopt->hicredit * 1024 * 8;
+ priv->plat->tx_queues_cfg[queue].high_credit = value & GENMASK(31, 0);
+
+ value = qopt->locredit * 1024 * 8;
+ priv->plat->tx_queues_cfg[queue].low_credit = value & GENMASK(31, 0);
+
+ ret = stmmac_config_cbs(priv, priv->hw,
+ priv->plat->tx_queues_cfg[queue].send_slope,
+ priv->plat->tx_queues_cfg[queue].idle_slope,
+ priv->plat->tx_queues_cfg[queue].high_credit,
+ priv->plat->tx_queues_cfg[queue].low_credit,
+ queue);
+ if (ret)
+ return ret;
+
+ dev_info(priv->device, "CBS queue %d: send %d, idle %d, hi %d, lo %d\n",
+ queue, qopt->sendslope, qopt->idleslope,
+ qopt->hicredit, qopt->locredit);
+ return 0;
+}
+
const struct stmmac_tc_ops dwmac510_tc_ops = {
.init = tc_init,
.setup_cls_u32 = tc_setup_cls_u32,
+ .setup_cbs = tc_setup_cbs,
};
--
2.7.4
^ permalink raw reply related
* Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
From: Daniel Borkmann @ 2018-06-27 12:56 UTC (permalink / raw)
To: Okash Khawaja
Cc: Jakub Kicinski, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, David S. Miller, netdev,
kernel-team, linux-kernel
In-Reply-To: <20180627114737.GA1580@w1t1fb>
On 06/27/2018 01:47 PM, Okash Khawaja wrote:
> On Wed, Jun 27, 2018 at 12:34:35PM +0200, Daniel Borkmann wrote:
>> On 06/27/2018 12:35 AM, Jakub Kicinski wrote:
>>> On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:
>>>> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:
>> [...]
>>>>> Implementing both outputs in one series will help you structure your
>>>>> code to best suit both of the formats up front.
>>>> hex and "formatted" are the only things missing? As always, things
>>>> can be refactored when new use case comes up. Lets wait for
>>>> Okash input.
>>>>
>>>> Regardless, plaintext is our current use case. Having the current
>>>> patchset in does not stop us or others from contributing other use
>>>> cases (json, "bpftool map find"...etc), and IMO it is actually
>>>> the opposite. Others may help us get there faster than us alone.
>>>> We should not stop making forward progress and take this patch
>>>> as hostage because "abc" and "xyz" are not done together.
>>>
>>> Parity between JSON and plain text output is non negotiable.
>>
>> Longish discussion and some confusion in this thread. :-) First of all
>> thanks a lot for working on it, very useful!
> Thanks :)
>
>> My $0.02 on it is that so far
>> great care has been taken in bpftool to indeed have feature parity between
>> JSON and plain text, so it would be highly desirable to keep continuing
>> this practice if the consensus is that it indeed is feasible and makes
>> sense wrt BTF data. There has been mentioned that given BTF data can be
>> dynamic depending on what the user loads via bpf(2) so a potential JSON
>> output may look different/break each time anyway. This however could all be
>> embedded under a container object that has a fixed key like 'formatted'
>> where tools like jq(1) can query into it. I think this would be fine since
>> the rest of the (non-dynamic) output is still retained as-is and then
>> wouldn't confuse or collide with existing users, and anyone programmatically
>> parsing deeper into the BTF data under such JSON container object needs
>> to have awareness of what specific data it wants to query from it; so
>> there's no conflict wrt breaking anything here. Imho, both outputs would
>> be very valuable.
> Okay I can add "formatted" object under json output.
>
> One thing to note here is that the fixed output will change if the map
> itself changes. So someone writing a program that consumes that fixed
> output will have to account for his program breaking in future, thus
Yes, that aspect is fine though, any program/script parsing this would need
to be aware of the underlying map type to make sense of it (e.g. per-cpu vs
non per-cpu maps to name one). But that info it could query/verify already
beforehand via bpftool as well (via normal map info dump for a given id).
> breaking backward compatibility anyway as far as the developer is
> concerned :)
>
> I will go ahead with work on "formatted" object.
Cool, thanks,
Daniel
^ permalink raw reply
* Re: [PATCH] infiniband: i40iw, nes: don't use wall time for TCP sequence numbers
From: Arnd Bergmann @ 2018-06-27 13:24 UTC (permalink / raw)
To: Shiraz Saleem
Cc: Ismail, Mustafa, Kees Cook, Latif, Faisal, y2038@lists.linaro.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Yuval Shaia,
Orosco, Henry, Jason Gunthorpe, Doug Ledford, Jia-Ju Bai,
Nikolova, Tatyana E, Bart Van Assche, David S. Miller,
Reshetova, Elena, linux-rdma@vger.kernel.org
In-Reply-To: <20180623000137.GA17264@ssaleem-MOBL4.amr.corp.intel.com>
On Sat, Jun 23, 2018 at 2:01 AM, Shiraz Saleem <shiraz.saleem@intel.com> wrote:
>> @@ -2164,7 +2165,6 @@ static struct i40iw_cm_node *i40iw_make_cm_node(
>> struct i40iw_cm_listener *listener)
>> {
>> struct i40iw_cm_node *cm_node;
>> - struct timespec ts;
>> int oldarpindex;
>> int arpindex;
>> struct net_device *netdev = iwdev->netdev;
>> @@ -2214,8 +2214,10 @@ static struct i40iw_cm_node *i40iw_make_cm_node(
>> cm_node->tcp_cntxt.rcv_wscale = I40IW_CM_DEFAULT_RCV_WND_SCALE;
>> cm_node->tcp_cntxt.rcv_wnd =
>> I40IW_CM_DEFAULT_RCV_WND_SCALED >> I40IW_CM_DEFAULT_RCV_WND_SCALE;
>> - ts = current_kernel_time();
>> - cm_node->tcp_cntxt.loc_seq_num = ts.tv_nsec;
>> + cm_node->tcp_cntxt.loc_seq_num = secure_tcp_seq(htonl(cm_node->loc_addr[0]),
>> + htonl(cm_node->rem_addr[0]),
>> + htons(cm_node->loc_port),
>> + htons(cm_node->rem_port));
>
> Should we not be using secure_tcpv6_seq() when we are ipv6?
I had not realized that there is a difference, but yes, from looking
at that function it seems that we should. v2 coming now.
Arnd
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
^ permalink raw reply
* [PATCH] [v2] infiniband: i40iw, nes: don't use wall time for TCP sequence numbers
From: Arnd Bergmann @ 2018-06-27 13:26 UTC (permalink / raw)
To: Faisal Latif, Shiraz Saleem, Doug Ledford, Jason Gunthorpe,
David S. Miller
Cc: Arnd Bergmann, Henry Orosco, Tatyana Nikolova, Mustafa Ismail,
Bart Van Assche, Yuval Shaia, linux-rdma, linux-kernel, netdev
The nes infiniband driver uses current_kernel_time() to get a nanosecond
granunarity timestamp to initialize its tcp sequence counters. This is
one of only a few remaining users of that deprecated function, so we
should try to get rid of it.
Aside from using a deprecated API, there are several problems I see here:
- Using a CLOCK_REALTIME based time source makes it predictable in
case the time base is synchronized.
- Using a coarse timestamp means it only gets updated once per jiffie,
making it even more predictable in order to avoid having to access
the hardware clock source
- The upper 2 bits are always zero because the nanoseconds are at most
999999999.
For the Linux TCP implementation, we use secure_tcp_seq(), which appears
to be appropriate here as well, and solves all the above problems.
i40iw uses a variant of the same code, so I do that same thing there
for ipv4. Unlike nes, i40e also supports ipv6, which needs to call
secure_tcpv6_seq instead.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: use secure_tcpv6_seq for IPv6 support as suggested by Shiraz Saleem.
---
drivers/infiniband/hw/i40iw/i40iw_cm.c | 26 +++++++++++++++++++++-----
drivers/infiniband/hw/nes/nes_cm.c | 8 +++++---
net/core/secure_seq.c | 1 +
3 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/drivers/infiniband/hw/i40iw/i40iw_cm.c b/drivers/infiniband/hw/i40iw/i40iw_cm.c
index 7b2655128b9f..e2590784b857 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_cm.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_cm.c
@@ -57,6 +57,7 @@
#include <net/addrconf.h>
#include <net/ip6_route.h>
#include <net/ip_fib.h>
+#include <net/secure_seq.h>
#include <net/tcp.h>
#include <asm/checksum.h>
@@ -2164,7 +2165,6 @@ static struct i40iw_cm_node *i40iw_make_cm_node(
struct i40iw_cm_listener *listener)
{
struct i40iw_cm_node *cm_node;
- struct timespec ts;
int oldarpindex;
int arpindex;
struct net_device *netdev = iwdev->netdev;
@@ -2214,10 +2214,26 @@ static struct i40iw_cm_node *i40iw_make_cm_node(
cm_node->tcp_cntxt.rcv_wscale = I40IW_CM_DEFAULT_RCV_WND_SCALE;
cm_node->tcp_cntxt.rcv_wnd =
I40IW_CM_DEFAULT_RCV_WND_SCALED >> I40IW_CM_DEFAULT_RCV_WND_SCALE;
- ts = current_kernel_time();
- cm_node->tcp_cntxt.loc_seq_num = ts.tv_nsec;
- cm_node->tcp_cntxt.mss = (cm_node->ipv4) ? (iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV4) :
- (iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV6);
+ if (cm_node->ipv4) {
+ cm_node->tcp_cntxt.loc_seq_num = secure_tcp_seq(htonl(cm_node->loc_addr[0]),
+ htonl(cm_node->rem_addr[0]),
+ htons(cm_node->loc_port),
+ htons(cm_node->rem_port));
+ cm_node->tcp_cntxt.mss = iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV4;
+ } else {
+ __be32 loc[4] = {
+ htonl(cm_node->loc_addr[0]), htonl(cm_node->loc_addr[1]),
+ htonl(cm_node->loc_addr[2]), htonl(cm_node->loc_addr[3])
+ };
+ __be32 rem[4] = {
+ htonl(cm_node->rem_addr[0]), htonl(cm_node->rem_addr[1]),
+ htonl(cm_node->rem_addr[2]), htonl(cm_node->rem_addr[3])
+ };
+ cm_node->tcp_cntxt.loc_seq_num = secure_tcpv6_seq(loc, rem,
+ htons(cm_node->loc_port),
+ htons(cm_node->rem_port));
+ cm_node->tcp_cntxt.mss = iwdev->vsi.mtu - I40IW_MTU_TO_MSS_IPV6;
+ }
cm_node->iwdev = iwdev;
cm_node->dev = &iwdev->sc_dev;
diff --git a/drivers/infiniband/hw/nes/nes_cm.c b/drivers/infiniband/hw/nes/nes_cm.c
index 6cdfbf8c5674..2b67ace5b614 100644
--- a/drivers/infiniband/hw/nes/nes_cm.c
+++ b/drivers/infiniband/hw/nes/nes_cm.c
@@ -58,6 +58,7 @@
#include <net/neighbour.h>
#include <net/route.h>
#include <net/ip_fib.h>
+#include <net/secure_seq.h>
#include <net/tcp.h>
#include <linux/fcntl.h>
@@ -1445,7 +1446,6 @@ static struct nes_cm_node *make_cm_node(struct nes_cm_core *cm_core,
struct nes_cm_listener *listener)
{
struct nes_cm_node *cm_node;
- struct timespec ts;
int oldarpindex = 0;
int arpindex = 0;
struct nes_device *nesdev;
@@ -1496,8 +1496,10 @@ static struct nes_cm_node *make_cm_node(struct nes_cm_core *cm_core,
cm_node->tcp_cntxt.rcv_wscale = NES_CM_DEFAULT_RCV_WND_SCALE;
cm_node->tcp_cntxt.rcv_wnd = NES_CM_DEFAULT_RCV_WND_SCALED >>
NES_CM_DEFAULT_RCV_WND_SCALE;
- ts = current_kernel_time();
- cm_node->tcp_cntxt.loc_seq_num = htonl(ts.tv_nsec);
+ cm_node->tcp_cntxt.loc_seq_num = secure_tcp_seq(htonl(cm_node->loc_addr),
+ htonl(cm_node->rem_addr),
+ htons(cm_node->loc_port),
+ htons(cm_node->rem_port));
cm_node->tcp_cntxt.mss = nesvnic->max_frame_size - sizeof(struct iphdr) -
sizeof(struct tcphdr) - ETH_HLEN - VLAN_HLEN;
cm_node->tcp_cntxt.rcv_nxt = 0;
diff --git a/net/core/secure_seq.c b/net/core/secure_seq.c
index 7232274de334..af6ad467ed61 100644
--- a/net/core/secure_seq.c
+++ b/net/core/secure_seq.c
@@ -140,6 +140,7 @@ u32 secure_tcp_seq(__be32 saddr, __be32 daddr,
&net_secret);
return seq_scale(hash);
}
+EXPORT_SYMBOL_GPL(secure_tcp_seq);
u32 secure_ipv4_port_ephemeral(__be32 saddr, __be32 daddr, __be16 dport)
{
--
2.9.0
^ permalink raw reply related
* [PATCH v2 net-next 0/2] net: preserve sock reference when scrubbing the skb.
From: Flavio Leitner @ 2018-06-27 13:34 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal,
netfilter-devel, Flavio Leitner
The sock reference is lost when scrubbing the packet and that breaks
TSQ (TCP Small Queues) and XPS (Transmit Packet Steering) causing
performance impacts of about 50% in a single TCP stream when crossing
network namespaces.
XPS breaks because the queue mapping stored in the socket is not
available, so another random queue might be selected when the stack
needs to transmit something like a TCP ACK, or TCP Retransmissions.
That causes packet re-ordering and/or performance issues.
TSQ breaks because it orphans the packet while it is still in the
host, so packets are queued contributing to the buffer bloat problem.
Preserving the sock reference fixes both issues. The socket is
orphaned anyways in the receiving path before any relevant action,
but the transmit side needs some extra checking included in the
first patch.
The first patch will update netfilter to check if the socket
netns is local before use it.
The second patch removes the skb_orphan() from the skb_scrub_packet()
and improve the documentation.
ChangeLog:
- split into two (Eric)
- addressed Paolo's offline feedback to swap the checks in xt_socket.c
to preserve original behavior.
- improved ip-sysctl.txt (reported by Cong)
Flavio Leitner (2):
netfilter: check if the socket netns is correct.
skbuff: preserve sock reference when scrubbing the skb.
Documentation/networking/ip-sysctl.txt | 10 +++++-----
include/net/netfilter/nf_log.h | 3 ++-
net/core/skbuff.c | 1 -
net/ipv4/netfilter/nf_log_ipv4.c | 8 ++++----
net/ipv6/netfilter/nf_log_ipv6.c | 8 ++++----
net/netfilter/nf_conntrack_broadcast.c | 2 +-
net/netfilter/nf_log_common.c | 5 +++--
net/netfilter/nf_nat_core.c | 6 +++++-
net/netfilter/nft_meta.c | 9 ++++++---
net/netfilter/nft_socket.c | 5 ++++-
net/netfilter/xt_cgroup.c | 6 ++++--
net/netfilter/xt_owner.c | 2 +-
net/netfilter/xt_recent.c | 3 ++-
net/netfilter/xt_socket.c | 8 ++++++++
14 files changed, 49 insertions(+), 27 deletions(-)
--
2.14.3
^ permalink raw reply
* [PATCH v2 net-next 1/2] netfilter: check if the socket netns is correct.
From: Flavio Leitner @ 2018-06-27 13:34 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal,
netfilter-devel, Flavio Leitner
In-Reply-To: <20180627133426.3858-1-fbl@redhat.com>
Netfilter assumes that if the socket is present in the skb, then
it can be used because that reference is cleaned up while the skb
is crossing netns.
We want to change that to preserve the socket reference in a future
patch, so this is a preparation updating netfilter to check if the
socket netns matches before use it.
Signed-off-by: Flavio Leitner <fbl@redhat.com>
---
include/net/netfilter/nf_log.h | 3 ++-
net/ipv4/netfilter/nf_log_ipv4.c | 8 ++++----
net/ipv6/netfilter/nf_log_ipv6.c | 8 ++++----
net/netfilter/nf_conntrack_broadcast.c | 2 +-
net/netfilter/nf_log_common.c | 5 +++--
net/netfilter/nf_nat_core.c | 6 +++++-
net/netfilter/nft_meta.c | 9 ++++++---
net/netfilter/nft_socket.c | 5 ++++-
net/netfilter/xt_cgroup.c | 6 ++++--
net/netfilter/xt_owner.c | 2 +-
net/netfilter/xt_recent.c | 3 ++-
net/netfilter/xt_socket.c | 8 ++++++++
12 files changed, 44 insertions(+), 21 deletions(-)
diff --git a/include/net/netfilter/nf_log.h b/include/net/netfilter/nf_log.h
index e811ac07ea94..0d3920896d50 100644
--- a/include/net/netfilter/nf_log.h
+++ b/include/net/netfilter/nf_log.h
@@ -106,7 +106,8 @@ int nf_log_dump_udp_header(struct nf_log_buf *m, const struct sk_buff *skb,
int nf_log_dump_tcp_header(struct nf_log_buf *m, const struct sk_buff *skb,
u8 proto, int fragment, unsigned int offset,
unsigned int logflags);
-void nf_log_dump_sk_uid_gid(struct nf_log_buf *m, struct sock *sk);
+void nf_log_dump_sk_uid_gid(struct net *net, struct nf_log_buf *m,
+ struct sock *sk);
void nf_log_dump_packet_common(struct nf_log_buf *m, u_int8_t pf,
unsigned int hooknum, const struct sk_buff *skb,
const struct net_device *in,
diff --git a/net/ipv4/netfilter/nf_log_ipv4.c b/net/ipv4/netfilter/nf_log_ipv4.c
index 4388de0e5380..1e6f28c97d3a 100644
--- a/net/ipv4/netfilter/nf_log_ipv4.c
+++ b/net/ipv4/netfilter/nf_log_ipv4.c
@@ -35,7 +35,7 @@ static const struct nf_loginfo default_loginfo = {
};
/* One level of recursion won't kill us */
-static void dump_ipv4_packet(struct nf_log_buf *m,
+static void dump_ipv4_packet(struct net *net, struct nf_log_buf *m,
const struct nf_loginfo *info,
const struct sk_buff *skb, unsigned int iphoff)
{
@@ -183,7 +183,7 @@ static void dump_ipv4_packet(struct nf_log_buf *m,
/* Max length: 3+maxlen */
if (!iphoff) { /* Only recurse once. */
nf_log_buf_add(m, "[");
- dump_ipv4_packet(m, info, skb,
+ dump_ipv4_packet(net, m, info, skb,
iphoff + ih->ihl*4+sizeof(_icmph));
nf_log_buf_add(m, "] ");
}
@@ -251,7 +251,7 @@ static void dump_ipv4_packet(struct nf_log_buf *m,
/* Max length: 15 "UID=4294967295 " */
if ((logflags & NF_LOG_UID) && !iphoff)
- nf_log_dump_sk_uid_gid(m, skb->sk);
+ nf_log_dump_sk_uid_gid(net, m, skb->sk);
/* Max length: 16 "MARK=0xFFFFFFFF " */
if (!iphoff && skb->mark)
@@ -333,7 +333,7 @@ static void nf_log_ip_packet(struct net *net, u_int8_t pf,
if (in != NULL)
dump_ipv4_mac_header(m, loginfo, skb);
- dump_ipv4_packet(m, loginfo, skb, 0);
+ dump_ipv4_packet(net, m, loginfo, skb, 0);
nf_log_buf_close(m);
}
diff --git a/net/ipv6/netfilter/nf_log_ipv6.c b/net/ipv6/netfilter/nf_log_ipv6.c
index b397a8fe88b9..c6bf580d0f33 100644
--- a/net/ipv6/netfilter/nf_log_ipv6.c
+++ b/net/ipv6/netfilter/nf_log_ipv6.c
@@ -36,7 +36,7 @@ static const struct nf_loginfo default_loginfo = {
};
/* One level of recursion won't kill us */
-static void dump_ipv6_packet(struct nf_log_buf *m,
+static void dump_ipv6_packet(struct net *net, struct nf_log_buf *m,
const struct nf_loginfo *info,
const struct sk_buff *skb, unsigned int ip6hoff,
int recurse)
@@ -258,7 +258,7 @@ static void dump_ipv6_packet(struct nf_log_buf *m,
/* Max length: 3+maxlen */
if (recurse) {
nf_log_buf_add(m, "[");
- dump_ipv6_packet(m, info, skb,
+ dump_ipv6_packet(net, m, info, skb,
ptr + sizeof(_icmp6h), 0);
nf_log_buf_add(m, "] ");
}
@@ -278,7 +278,7 @@ static void dump_ipv6_packet(struct nf_log_buf *m,
/* Max length: 15 "UID=4294967295 " */
if ((logflags & NF_LOG_UID) && recurse)
- nf_log_dump_sk_uid_gid(m, skb->sk);
+ nf_log_dump_sk_uid_gid(net, m, skb->sk);
/* Max length: 16 "MARK=0xFFFFFFFF " */
if (recurse && skb->mark)
@@ -365,7 +365,7 @@ static void nf_log_ip6_packet(struct net *net, u_int8_t pf,
if (in != NULL)
dump_ipv6_mac_header(m, loginfo, skb);
- dump_ipv6_packet(m, loginfo, skb, skb_network_offset(skb), 1);
+ dump_ipv6_packet(net, m, loginfo, skb, skb_network_offset(skb), 1);
nf_log_buf_close(m);
}
diff --git a/net/netfilter/nf_conntrack_broadcast.c b/net/netfilter/nf_conntrack_broadcast.c
index a1086bdec242..5423b197d98a 100644
--- a/net/netfilter/nf_conntrack_broadcast.c
+++ b/net/netfilter/nf_conntrack_broadcast.c
@@ -32,7 +32,7 @@ int nf_conntrack_broadcast_help(struct sk_buff *skb,
__be32 mask = 0;
/* we're only interested in locally generated packets */
- if (skb->sk == NULL)
+ if (skb->sk == NULL || !net_eq(nf_ct_net(ct), sock_net(skb->sk)))
goto out;
if (rt == NULL || !(rt->rt_flags & RTCF_BROADCAST))
goto out;
diff --git a/net/netfilter/nf_log_common.c b/net/netfilter/nf_log_common.c
index dc61399e30be..a8c5c846aec1 100644
--- a/net/netfilter/nf_log_common.c
+++ b/net/netfilter/nf_log_common.c
@@ -132,9 +132,10 @@ int nf_log_dump_tcp_header(struct nf_log_buf *m, const struct sk_buff *skb,
}
EXPORT_SYMBOL_GPL(nf_log_dump_tcp_header);
-void nf_log_dump_sk_uid_gid(struct nf_log_buf *m, struct sock *sk)
+void nf_log_dump_sk_uid_gid(struct net *net, struct nf_log_buf *m,
+ struct sock *sk)
{
- if (!sk || !sk_fullsock(sk))
+ if (!sk || !sk_fullsock(sk) || !net_eq(net, sock_net(sk)))
return;
read_lock_bh(&sk->sk_callback_lock);
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 46f9df99d276..86df2a1666fd 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -108,6 +108,7 @@ int nf_xfrm_me_harder(struct net *net, struct sk_buff *skb, unsigned int family)
struct flowi fl;
unsigned int hh_len;
struct dst_entry *dst;
+ struct sock *sk = skb->sk;
int err;
err = xfrm_decode_session(skb, &fl, family);
@@ -119,7 +120,10 @@ int nf_xfrm_me_harder(struct net *net, struct sk_buff *skb, unsigned int family)
dst = ((struct xfrm_dst *)dst)->route;
dst_hold(dst);
- dst = xfrm_lookup(net, dst, &fl, skb->sk, 0);
+ if (sk && !net_eq(net, sock_net(sk)))
+ sk = NULL;
+
+ dst = xfrm_lookup(net, dst, &fl, sk, 0);
if (IS_ERR(dst))
return PTR_ERR(dst);
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 1105a23bda5e..2b94dcc43456 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -107,7 +107,8 @@ static void nft_meta_get_eval(const struct nft_expr *expr,
break;
case NFT_META_SKUID:
sk = skb_to_full_sk(skb);
- if (!sk || !sk_fullsock(sk))
+ if (!sk || !sk_fullsock(sk) ||
+ !net_eq(nft_net(pkt), sock_net(sk)))
goto err;
read_lock_bh(&sk->sk_callback_lock);
@@ -123,7 +124,8 @@ static void nft_meta_get_eval(const struct nft_expr *expr,
break;
case NFT_META_SKGID:
sk = skb_to_full_sk(skb);
- if (!sk || !sk_fullsock(sk))
+ if (!sk || !sk_fullsock(sk) ||
+ !net_eq(nft_net(pkt), sock_net(sk)))
goto err;
read_lock_bh(&sk->sk_callback_lock);
@@ -214,7 +216,8 @@ static void nft_meta_get_eval(const struct nft_expr *expr,
#ifdef CONFIG_CGROUP_NET_CLASSID
case NFT_META_CGROUP:
sk = skb_to_full_sk(skb);
- if (!sk || !sk_fullsock(sk))
+ if (!sk || !sk_fullsock(sk) ||
+ !net_eq(nft_net(pkt), sock_net(sk)))
goto err;
*dest = sock_cgroup_classid(&sk->sk_cgrp_data);
break;
diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c
index 74e1b3bd6954..998c2b546f6d 100644
--- a/net/netfilter/nft_socket.c
+++ b/net/netfilter/nft_socket.c
@@ -23,6 +23,9 @@ static void nft_socket_eval(const struct nft_expr *expr,
struct sock *sk = skb->sk;
u32 *dest = ®s->data[priv->dreg];
+ if (sk && !net_eq(nft_net(pkt), sock_net(sk)))
+ sk = NULL;
+
if (!sk)
switch(nft_pf(pkt)) {
case NFPROTO_IPV4:
@@ -39,7 +42,7 @@ static void nft_socket_eval(const struct nft_expr *expr,
return;
}
- if(!sk) {
+ if (!sk) {
nft_reg_store8(dest, 0);
return;
}
diff --git a/net/netfilter/xt_cgroup.c b/net/netfilter/xt_cgroup.c
index 7df2dece57d3..5d92e1781980 100644
--- a/net/netfilter/xt_cgroup.c
+++ b/net/netfilter/xt_cgroup.c
@@ -72,8 +72,9 @@ static bool
cgroup_mt_v0(const struct sk_buff *skb, struct xt_action_param *par)
{
const struct xt_cgroup_info_v0 *info = par->matchinfo;
+ struct sock *sk = skb->sk;
- if (skb->sk == NULL || !sk_fullsock(skb->sk))
+ if (!sk || !sk_fullsock(sk) || !net_eq(xt_net(par), sock_net(sk)))
return false;
return (info->id == sock_cgroup_classid(&skb->sk->sk_cgrp_data)) ^
@@ -85,8 +86,9 @@ static bool cgroup_mt_v1(const struct sk_buff *skb, struct xt_action_param *par)
const struct xt_cgroup_info_v1 *info = par->matchinfo;
struct sock_cgroup_data *skcd = &skb->sk->sk_cgrp_data;
struct cgroup *ancestor = info->priv;
+ struct sock *sk = skb->sk;
- if (!skb->sk || !sk_fullsock(skb->sk))
+ if (!sk || !sk_fullsock(sk) || !net_eq(xt_net(par), sock_net(sk)))
return false;
if (ancestor)
diff --git a/net/netfilter/xt_owner.c b/net/netfilter/xt_owner.c
index 3d705c688a27..46686fb73784 100644
--- a/net/netfilter/xt_owner.c
+++ b/net/netfilter/xt_owner.c
@@ -67,7 +67,7 @@ owner_mt(const struct sk_buff *skb, struct xt_action_param *par)
struct sock *sk = skb_to_full_sk(skb);
struct net *net = xt_net(par);
- if (sk == NULL || sk->sk_socket == NULL)
+ if (!sk || !sk->sk_socket || !net_eq(net, sock_net(sk)))
return (info->match ^ info->invert) == 0;
else if (info->match & info->invert & XT_OWNER_SOCKET)
/*
diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index 07085c22b19c..f44de4bc2100 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -265,7 +265,8 @@ recent_mt(const struct sk_buff *skb, struct xt_action_param *par)
}
/* use TTL as seen before forwarding */
- if (xt_out(par) != NULL && skb->sk == NULL)
+ if (xt_out(par) != NULL &&
+ (!skb->sk || !net_eq(net, sock_net(skb->sk))))
ttl++;
spin_lock_bh(&recent_lock);
diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c
index 5c0779c4fa3c..0472f3472842 100644
--- a/net/netfilter/xt_socket.c
+++ b/net/netfilter/xt_socket.c
@@ -56,8 +56,12 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
struct sk_buff *pskb = (struct sk_buff *)skb;
struct sock *sk = skb->sk;
+ if (!net_eq(xt_net(par), sock_net(sk)))
+ sk = NULL;
+
if (!sk)
sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par));
+
if (sk) {
bool wildcard;
bool transparent = true;
@@ -113,8 +117,12 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par)
struct sk_buff *pskb = (struct sk_buff *)skb;
struct sock *sk = skb->sk;
+ if (!net_eq(xt_net(par), sock_net(sk)))
+ sk = NULL;
+
if (!sk)
sk = nf_sk_lookup_slow_v6(xt_net(par), skb, xt_in(par));
+
if (sk) {
bool wildcard;
bool transparent = true;
--
2.14.3
^ permalink raw reply related
* [PATCH v2 net-next 2/2] skbuff: preserve sock reference when scrubbing the skb.
From: Flavio Leitner @ 2018-06-27 13:34 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal,
netfilter-devel, Flavio Leitner
In-Reply-To: <20180627133426.3858-1-fbl@redhat.com>
The sock reference is lost when scrubbing the packet and that breaks
TSQ (TCP Small Queues) and XPS (Transmit Packet Steering) causing
performance impacts of about 50% in a single TCP stream when crossing
network namespaces.
XPS breaks because the queue mapping stored in the socket is not
available, so another random queue might be selected when the stack
needs to transmit something like a TCP ACK, or TCP Retransmissions.
That causes packet re-ordering and/or performance issues.
TSQ breaks because it orphans the packet while it is still in the
host, so packets are queued contributing to the buffer bloat problem.
Preserving the sock reference fixes both issues. The socket is
orphaned anyways in the receiving path before any relevant action
and on TX side the netfilter checks if the reference is local before
use it.
Signed-off-by: Flavio Leitner <fbl@redhat.com>
---
Documentation/networking/ip-sysctl.txt | 10 +++++-----
net/core/skbuff.c | 1 -
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index ce8fbf5aa63c..f4c042be0216 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -733,11 +733,11 @@ tcp_limit_output_bytes - INTEGER
Controls TCP Small Queue limit per tcp socket.
TCP bulk sender tends to increase packets in flight until it
gets losses notifications. With SNDBUF autotuning, this can
- result in a large amount of packets queued in qdisc/device
- on the local machine, hurting latency of other flows, for
- typical pfifo_fast qdiscs.
- tcp_limit_output_bytes limits the number of bytes on qdisc
- or device to reduce artificial RTT/cwnd and reduce bufferbloat.
+ result in a large amount of packets queued on the local machine
+ (e.g.: qdiscs, CPU backlog, or device) hurting latency of other
+ flows, for typical pfifo_fast qdiscs. tcp_limit_output_bytes
+ limits the number of bytes on qdisc or device to reduce artificial
+ RTT/cwnd and reduce bufferbloat.
Default: 262144
tcp_challenge_ack_limit - INTEGER
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b1f274f22d85..f59e98ca72c5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4911,7 +4911,6 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet)
return;
ipvs_reset(skb);
- skb_orphan(skb);
skb->mark = 0;
}
EXPORT_SYMBOL_GPL(skb_scrub_packet);
--
2.14.3
^ permalink raw reply related
* Re: [PATCH net-next] mlx4: do not use rwlock in fast path
From: Eric Dumazet @ 2018-06-27 13:49 UTC (permalink / raw)
To: Tariq Toukan, Eric Dumazet, David Miller
Cc: netdev, Shawn Bohrer, Shay Agroskin, Eran Ben Elisha
In-Reply-To: <05ae066b-873d-159b-4ac2-ab39120c949b@mellanox.com>
On 06/27/2018 05:11 AM, Tariq Toukan wrote:
>
>
> On 09/02/2017 7:10 PM, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> Using a reader-writer lock in fast path is silly, when we can
>> instead use RCU or a seqlock.
>>
>> For mlx4 hwstamp clock, a seqlock is the way to go, removing
>> two atomic operations and false sharing.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Cc: Tariq Toukan <tariqt@mellanox.com>
>> ---
>> drivers/net/ethernet/mellanox/mlx4/en_clock.c | 35 ++++++++--------
>> drivers/net/ethernet/mellanox/mlx4/mlx4_en.h | 2
>> 2 files changed, 19 insertions(+), 18 deletions(-)
>>
>
> Hi Eric,
>
> When my peer, Shay, modified mlx5 to adopt this same locking scheme/type, he noticed a degradation in packet rate.
> He got back to testing mlx4 and also noticed a degradation introduced by this patch.
>
> Perf numbers (single ring):
>
> mlx4:
> with rw-lock: ~8.54M pps
> with seq-lock: ~8.51M pps
>
> mlx5:
> With rw-lock: ~14.94M pps
> With seq-lock: ~14.48M pps
>
> Actually, this can be explained by the analysis below.
> In short, number of readers is significantly larger than of writers. Hence optimizing the readers flow would give better numbers. The issue is, the read/write lock might cause writers starvation. Maybe RCU fits best here?
>
> Degradation analysis:
> The patch changes the lock type which protects reads and updates of a variable ( (struct mlx4_en_dev).clock variable)
> This variable is used to convert the hw timestamp into skb->hwtstamps.
> This variable is read for each transmitted/received packet and updated only via ptp module and some overflow periodic work we have (maximum of 10 times per second)
> Meaning that there are much more readers than writers, and it’s best to optimize the readers flow.
>
Hi Tariq
Are you sure you enabled time stamps in your tests ?
mlx4_en_fill_hwtstamps() is _really_ called 8,540,000 times per second,
meaning a same amount of read_lock_irqsave()/read_unlock_irqrestore() is performed ?
You have a pretty damn good CPU it seems.
seqlock has no cost for a reader [1], other than reading one integer value and testing it.
[1] If this value never change (and is on a clean cache line).
Really this looks like ring->hwtstamp_rx_filter != HWTSTAMP_FILTER_ALL in your tests.
The numbers you gave just give one cycle difference per packet (half a nano second),
so I really doubt adding back the heavy read_lock_irqsave()/read_unlock_irqrestore()
could be faster.
Conceptually seqlock is some form of RCU, it really optimizes the readers flow.
Thanks
^ permalink raw reply
* Re: [PATCH v2 net-next 2/2] skbuff: preserve sock reference when scrubbing the skb.
From: Eric Dumazet @ 2018-06-27 13:56 UTC (permalink / raw)
To: Flavio Leitner, netdev
Cc: Eric Dumazet, Paolo Abeni, David Miller, Florian Westphal,
netfilter-devel
In-Reply-To: <20180627133426.3858-3-fbl@redhat.com>
On 06/27/2018 06:34 AM, Flavio Leitner wrote:
> The sock reference is lost when scrubbing the packet and that breaks
> TSQ (TCP Small Queues) and XPS (Transmit Packet Steering) causing
> performance impacts of about 50% in a single TCP stream when crossing
> network namespaces.
>
> XPS breaks because the queue mapping stored in the socket is not
> available, so another random queue might be selected when the stack
> needs to transmit something like a TCP ACK, or TCP Retransmissions.
> That causes packet re-ordering and/or performance issues.
Note we do not care if another random queue is selected when TCP retransmit
after timeout happens.
The problem is really when sending a normal train of packets (being retransmission
or not). We want all of them going through one queue to avoid reorders.
After an idle period (no packets are in any qdisc/NIC queue), we absolutely
are okay to select another "random queue".
This choice is driven by skb->ooo_okay
Most TCP ACK packets are sent while no prior packet is in qdisc, so should
have ooo_okay set to 1.
>
> TSQ breaks because it orphans the packet while it is still in the
> host, so packets are queued contributing to the buffer bloat problem.
>
> Preserving the sock reference fixes both issues. The socket is
> orphaned anyways in the receiving path before any relevant action
> and on TX side the netfilter checks if the reference is local before
> use it.
>
> Signed-off-by: Flavio Leitner <fbl@redhat.com>
> ---
> Documentation/networking/ip-sysctl.txt | 10 +++++-----
> net/core/skbuff.c | 1 -
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
> index ce8fbf5aa63c..f4c042be0216 100644
> --- a/Documentation/networking/ip-sysctl.txt
> +++ b/Documentation/networking/ip-sysctl.txt
> @@ -733,11 +733,11 @@ tcp_limit_output_bytes - INTEGER
> Controls TCP Small Queue limit per tcp socket.
> TCP bulk sender tends to increase packets in flight until it
> gets losses notifications. With SNDBUF autotuning, this can
> - result in a large amount of packets queued in qdisc/device
> - on the local machine, hurting latency of other flows, for
> - typical pfifo_fast qdiscs.
> - tcp_limit_output_bytes limits the number of bytes on qdisc
> - or device to reduce artificial RTT/cwnd and reduce bufferbloat.
> + result in a large amount of packets queued on the local machine
> + (e.g.: qdiscs, CPU backlog, or device) hurting latency of other
> + flows, for typical pfifo_fast qdiscs. tcp_limit_output_bytes
> + limits the number of bytes on qdisc or device to reduce artificial
> + RTT/cwnd and reduce bufferbloat.
> Default: 262144
>
> tcp_challenge_ack_limit - INTEGER
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index b1f274f22d85..f59e98ca72c5 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4911,7 +4911,6 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet)
> return;
>
> ipvs_reset(skb);
> - skb_orphan(skb);
> skb->mark = 0;
> }
> EXPORT_SYMBOL_GPL(skb_scrub_packet);
>
^ permalink raw reply
* [PATCH bpf 2/4] xsk: frame could be completed more than once in SKB path
From: Magnus Karlsson @ 2018-06-27 14:02 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev; +Cc: qi.z.zhang, pavel
In-Reply-To: <1530108136-4984-1-git-send-email-magnus.karlsson@intel.com>
Fixed a bug in which a frame could be completed more than once
when an error was returned from dev_direct_xmit(). The code
erroneously retried sending the message leading to multiple
calls to the SKB destructor and therefore multiple completions
of the same buffer to user space.
The error code in this case has been changed from EAGAIN to EBUSY
in order to tell user space that the sending of the packet failed
and the buffer has been return to user space through the completion
queue.
Fixes: 35fcde7f8deb ("xsk: support for Tx")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Pavel Odintsov <pavel@fastnetmon.com>
---
net/xdp/xsk.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 3b3410ada097..d482f727f4c2 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -268,15 +268,15 @@ static int xsk_generic_xmit(struct sock *sk, struct msghdr *m,
skb->destructor = xsk_destruct_skb;
err = dev_direct_xmit(skb, xs->queue_id);
+ xskq_discard_desc(xs->tx);
/* Ignore NET_XMIT_CN as packet might have been sent */
if (err == NET_XMIT_DROP || err == NETDEV_TX_BUSY) {
- err = -EAGAIN;
- /* SKB consumed by dev_direct_xmit() */
+ /* SKB completed but not sent */
+ err = -EBUSY;
goto out;
}
sent_frame = true;
- xskq_discard_desc(xs->tx);
}
out:
--
2.7.4
^ permalink raw reply related
* [PATCH bpf 3/4] samples/bpf: deal with EBUSY return code from sendmsg in xdpsock sample
From: Magnus Karlsson @ 2018-06-27 14:02 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev; +Cc: qi.z.zhang, pavel
In-Reply-To: <1530108136-4984-1-git-send-email-magnus.karlsson@intel.com>
Sendmsg in the SKB path of AF_XDP can now return EBUSY when a packet
was discarded and completed by the driver. Just ignore this message
in the sample application.
Fixes: b4b8faa1ded7 ("samples/bpf: sample application and documentation for AF_XDP sockets")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Pavel Odintsov <pavel@fastnetmon.com>
---
samples/bpf/xdpsock_user.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/samples/bpf/xdpsock_user.c b/samples/bpf/xdpsock_user.c
index d69c8d78d3fd..aec3a61fac44 100644
--- a/samples/bpf/xdpsock_user.c
+++ b/samples/bpf/xdpsock_user.c
@@ -729,7 +729,7 @@ static void kick_tx(int fd)
int ret;
ret = sendto(fd, NULL, 0, MSG_DONTWAIT, NULL, 0);
- if (ret >= 0 || errno == ENOBUFS || errno == EAGAIN)
+ if (ret >= 0 || errno == EAGAIN || errno == EBUSY)
return;
lassert(0);
}
--
2.7.4
^ permalink raw reply related
* [PATCH bpf 0/4] Bug fixes to the SKB TX path of AF_XDP
From: Magnus Karlsson @ 2018-06-27 14:02 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev; +Cc: qi.z.zhang, pavel
This patch set fixes three bugs in the SKB TX path of AF_XDP.
Details in the individual commits.
The structure of the patch set is as follows:
Patch 1: Fix for lost completion message
Patch 2-3: Fix for possible multiple completions of single packet
Patch 4: Fix potential race during error
/Magnus
Magnus Karlsson (4):
xsk: fix potential lost completion message in SKB path
xsk: frame could be completed more than once in SKB path
samples/bpf: deal with EBUSY return code from sendmsg in xdpsock
sample
xsk: fix potential race in SKB TX completion code
include/net/xdp_sock.h | 4 ++++
net/xdp/xsk.c | 10 +++++++---
net/xdp/xsk_queue.h | 9 ++-------
samples/bpf/xdpsock_user.c | 2 +-
4 files changed, 14 insertions(+), 11 deletions(-)
^ permalink raw reply
* [PATCH bpf 1/4] xsk: fix potential lost completion message in SKB path
From: Magnus Karlsson @ 2018-06-27 14:02 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev; +Cc: qi.z.zhang, pavel
In-Reply-To: <1530108136-4984-1-git-send-email-magnus.karlsson@intel.com>
The code in xskq_produce_addr erroneously checked if there
was up to LAZY_UPDATE_THRESHOLD amount of space in the completion
queue. It only needs to check if there is one slot left in the
queue. This bug could under some circumstances lead to a WARN_ON_ONCE
being triggered and the completion message to user space being lost.
Fixes: 35fcde7f8deb ("xsk: support for Tx")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
Reported-by: Pavel Odintsov <pavel@fastnetmon.com>
---
net/xdp/xsk_queue.h | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index ef6a6f0ec949..52ecaf770642 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -62,14 +62,9 @@ static inline u32 xskq_nb_avail(struct xsk_queue *q, u32 dcnt)
return (entries > dcnt) ? dcnt : entries;
}
-static inline u32 xskq_nb_free_lazy(struct xsk_queue *q, u32 producer)
-{
- return q->nentries - (producer - q->cons_tail);
-}
-
static inline u32 xskq_nb_free(struct xsk_queue *q, u32 producer, u32 dcnt)
{
- u32 free_entries = xskq_nb_free_lazy(q, producer);
+ u32 free_entries = q->nentries - (producer - q->cons_tail);
if (free_entries >= dcnt)
return free_entries;
@@ -129,7 +124,7 @@ static inline int xskq_produce_addr(struct xsk_queue *q, u64 addr)
{
struct xdp_umem_ring *ring = (struct xdp_umem_ring *)q->ring;
- if (xskq_nb_free(q, q->prod_tail, LAZY_UPDATE_THRESHOLD) == 0)
+ if (xskq_nb_free(q, q->prod_tail, 1) == 0)
return -ENOSPC;
ring->desc[q->prod_tail++ & q->ring_mask] = addr;
--
2.7.4
^ permalink raw reply related
* [PATCH bpf 4/4] xsk: fix potential race in SKB TX completion code
From: Magnus Karlsson @ 2018-06-27 14:02 UTC (permalink / raw)
To: magnus.karlsson, bjorn.topel, ast, daniel, netdev; +Cc: qi.z.zhang, pavel
In-Reply-To: <1530108136-4984-1-git-send-email-magnus.karlsson@intel.com>
There was a potential race in the TX completion code for
the SKB case when the TX napi thread and the error path
of the sendmsg code could both call the SKB destructor
at the same time. Fixed by introducing a spin_lock in the
destructor.
Fixes: 35fcde7f8deb ("xsk: support for Tx")
Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
include/net/xdp_sock.h | 4 ++++
net/xdp/xsk.c | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index 9fe472f2ac95..7161856bcf9c 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -60,6 +60,10 @@ struct xdp_sock {
bool zc;
/* Protects multiple processes in the control path */
struct mutex mutex;
+ /* Mutual exclusion of NAPI TX thread and sendmsg error paths
+ * in the SKB destructor callback.
+ */
+ spinlock_t tx_completion_lock;
u64 rx_dropped;
};
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index d482f727f4c2..650c4da8dc5a 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -199,8 +199,11 @@ static void xsk_destruct_skb(struct sk_buff *skb)
{
u64 addr = (u64)(long)skb_shinfo(skb)->destructor_arg;
struct xdp_sock *xs = xdp_sk(skb->sk);
+ unsigned long flags;
+ spin_lock_irqsave(&xs->tx_completion_lock, flags);
WARN_ON_ONCE(xskq_produce_addr(xs->umem->cq, addr));
+ spin_unlock_irqrestore(&xs->tx_completion_lock, flags);
sock_wfree(skb);
}
@@ -754,6 +757,7 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
xs = xdp_sk(sk);
mutex_init(&xs->mutex);
+ spin_lock_init(&xs->tx_completion_lock);
local_bh_disable();
sock_prot_inuse_add(net, &xsk_proto, 1);
--
2.7.4
^ permalink raw reply related
* [PATCH net] net: stmmac: Set DMA buffer size in HW
From: Jose Abreu @ 2018-06-27 14:03 UTC (permalink / raw)
To: netdev
Cc: Jose Abreu, David S. Miller, Joao Pinto, Vitor Soares,
Giuseppe Cavallaro, Alexandre Torgue
This is clearly a bug.
We need to set the DMA buffer size in the HW otherwise corruption can
occur when receiving packets.
This is probably not occuring because of small MTU values and because HW
has a default value internally (which currently is bigger than default
buffer size).
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Joao Pinto <jpinto@synopsys.com>
Cc: Vitor Soares <soares@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
---
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 12 ++++++++++++
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h | 2 ++
drivers/net/ethernet/stmicro/stmmac/hwif.h | 3 +++
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 ++
4 files changed, 19 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index d37f17ca62fe..65bc3556bd8f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -407,6 +407,16 @@ static void dwmac4_enable_tso(void __iomem *ioaddr, bool en, u32 chan)
}
}
+static void dwmac4_set_bfsize(void __iomem *ioaddr, int bfsize, u32 chan)
+{
+ u32 value = readl(ioaddr + DMA_CHAN_RX_CONTROL(chan));
+
+ value &= ~DMA_RBSZ_MASK;
+ value |= (bfsize << DMA_RBSZ_SHIFT) & DMA_RBSZ_MASK;
+
+ writel(value, ioaddr + DMA_CHAN_RX_CONTROL(chan));
+}
+
const struct stmmac_dma_ops dwmac4_dma_ops = {
.reset = dwmac4_dma_reset,
.init = dwmac4_dma_init,
@@ -431,6 +441,7 @@ const struct stmmac_dma_ops dwmac4_dma_ops = {
.set_rx_tail_ptr = dwmac4_set_rx_tail_ptr,
.set_tx_tail_ptr = dwmac4_set_tx_tail_ptr,
.enable_tso = dwmac4_enable_tso,
+ .set_bfsize = dwmac4_set_bfsize,
};
const struct stmmac_dma_ops dwmac410_dma_ops = {
@@ -457,4 +468,5 @@ const struct stmmac_dma_ops dwmac410_dma_ops = {
.set_rx_tail_ptr = dwmac4_set_rx_tail_ptr,
.set_tx_tail_ptr = dwmac4_set_tx_tail_ptr,
.enable_tso = dwmac4_enable_tso,
+ .set_bfsize = dwmac4_set_bfsize,
};
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
index c63c1fe3f26b..22a4a6dbb1a4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
@@ -120,6 +120,8 @@
/* DMA Rx Channel X Control register defines */
#define DMA_CONTROL_SR BIT(0)
+#define DMA_RBSZ_MASK GENMASK(14, 1)
+#define DMA_RBSZ_SHIFT 1
/* Interrupt status per channel */
#define DMA_CHAN_STATUS_REB GENMASK(21, 19)
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index e44e7b26ce82..fe8b536b13f8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -183,6 +183,7 @@ struct stmmac_dma_ops {
void (*set_rx_tail_ptr)(void __iomem *ioaddr, u32 tail_ptr, u32 chan);
void (*set_tx_tail_ptr)(void __iomem *ioaddr, u32 tail_ptr, u32 chan);
void (*enable_tso)(void __iomem *ioaddr, bool en, u32 chan);
+ void (*set_bfsize)(void __iomem *ioaddr, int bfsize, u32 chan);
};
#define stmmac_reset(__priv, __args...) \
@@ -235,6 +236,8 @@ struct stmmac_dma_ops {
stmmac_do_void_callback(__priv, dma, set_tx_tail_ptr, __args)
#define stmmac_enable_tso(__priv, __args...) \
stmmac_do_void_callback(__priv, dma, enable_tso, __args)
+#define stmmac_set_dma_bfsize(__priv, __args...) \
+ stmmac_do_void_callback(__priv, dma, set_bfsize, __args)
struct mac_device_info;
struct net_device;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index cba46b62a1cd..60f59abab009 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1804,6 +1804,8 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv)
stmmac_dma_rx_mode(priv, priv->ioaddr, rxmode, chan,
rxfifosz, qmode);
+ stmmac_set_dma_bfsize(priv, priv->ioaddr, priv->dma_buf_sz,
+ chan);
}
for (chan = 0; chan < tx_channels_count; chan++) {
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox