* Re: [PATCH] tcp: detect use sendpage for slab-based objects
From: Vasily Averin @ 2019-02-25 9:32 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <56220566-0811-eabe-53f2-9fa625a58bbd@virtuozzo.com>
On 2/25/19 12:15 PM, Vasily Averin wrote:
> On 2/22/19 7:39 PM, Eric Dumazet wrote:
>> On Fri, Feb 22, 2019 at 6:02 AM Vasily Averin <vvs@virtuozzo.com> wrote:
>
>>> Eric, could you please elaborate once again why tcp_sendpage() should not handle slab objects?
>>
>> Simply because SLAB has its own way to manage objects from a page, and
>> does not care
>> about the underlying page having its refcount elevated.
>>
>> ptr = kmalloc(xx)
>> ... < here you can attempt cheating and add one to the underlying page>
>> kfree(ptr); // SLAB does not care of page count, it will effectively
>> put ptr in the free list.
>>
>> ptr2 = kmalloc(xx); //
>>
>> ptr2 can be the same than ptr (object was kfreed() earlier)
>>
>> This means that some other stuff will happily reuse the piece of
>> memory that you wanted to use for zero-copy.
>>
>> This is a serious bug IMO, since this would allow for data corruption.
>
> Thank you for explanation, however I still have some doubts.
>
> Yes, it's strange to use sendpage if we want to send some small 8-bytes-long slab based object,
> it's better to use sendmsg instead.
>
> Yes, using of sendpage for slab-based objects can require special attention
> to guarantee that slab object will not be freed until end of IO.
> However IMO this should be guaranteed if caller uses sendmsg instead of sendpage.
> Btw, as far as I understand in my example XFS did it correctly, submitted slab objects was kept in use
> and seems they should be freed after end of IO, via end_io callback.
> At least I did not found any bugs in sendpage callers.
>
> And most important, it seems for me switch from sendpage to sendmsg doe not resolve the problem completely:
> tcp_sendmsg_locked() under some conditions can merge neighbours slab-based tcp fragments,
> so local tcp_recvmsg() can trigger BUG_ON in this case too.
>
> Am I missed something probably?
Seems I missed that skb_copy_to_page_nocache() in tcp_sendpage_locked copies data from original slab object,
and merges fragments with copied data.
^ permalink raw reply
* Re: [PATCH] xfrm: correctly check policy index in verify_newpolicy_info
From: YueHaibing @ 2019-02-25 9:35 UTC (permalink / raw)
To: steffen.klassert, herbert, davem; +Cc: linux-kernel, netdev
In-Reply-To: <20190225092704.26796-1-yuehaibing@huawei.com>
Pls ignore this, sorry
On 2019/2/25 17:27, Yue Haibing wrote:
> From: YueHaibing <yuehaibing@huawei.com>
>
> UBSAN report this:
>
> UBSAN: Undefined behaviour in net/xfrm/xfrm_policy.c:1289:24
> index 6 is out of range for type 'unsigned int [6]'
> CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.4.162-514.55.6.9.x86_64+ #13
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> 0000000000000000 1466cf39b41b23c9 ffff8801f6b07a58 ffffffff81cb35f4
> 0000000041b58ab3 ffffffff83230f9c ffffffff81cb34e0 ffff8801f6b07a80
> ffff8801f6b07a20 1466cf39b41b23c9 ffffffff851706e0 ffff8801f6b07ae8
> Call Trace:
> <IRQ> [<ffffffff81cb35f4>] __dump_stack lib/dump_stack.c:15 [inline]
> <IRQ> [<ffffffff81cb35f4>] dump_stack+0x114/0x1a0 lib/dump_stack.c:51
> [<ffffffff81d94225>] ubsan_epilogue+0x12/0x8f lib/ubsan.c:164
> [<ffffffff81d954db>] __ubsan_handle_out_of_bounds+0x16e/0x1b2 lib/ubsan.c:382
> [<ffffffff82a25acd>] __xfrm_policy_unlink+0x3dd/0x5b0 net/xfrm/xfrm_policy.c:1289
> [<ffffffff82a2e572>] xfrm_policy_delete+0x52/0xb0 net/xfrm/xfrm_policy.c:1309
> [<ffffffff82a3319b>] xfrm_policy_timer+0x30b/0x590 net/xfrm/xfrm_policy.c:243
> [<ffffffff813d3927>] call_timer_fn+0x237/0x990 kernel/time/timer.c:1144
> [<ffffffff813d8e7e>] __run_timers kernel/time/timer.c:1218 [inline]
> [<ffffffff813d8e7e>] run_timer_softirq+0x6ce/0xb80 kernel/time/timer.c:1401
> [<ffffffff8120d6f9>] __do_softirq+0x299/0xe10 kernel/softirq.c:273
> [<ffffffff8120e676>] invoke_softirq kernel/softirq.c:350 [inline]
> [<ffffffff8120e676>] irq_exit+0x216/0x2c0 kernel/softirq.c:391
> [<ffffffff82c5edab>] exiting_irq arch/x86/include/asm/apic.h:652 [inline]
> [<ffffffff82c5edab>] smp_apic_timer_interrupt+0x8b/0xc0 arch/x86/kernel/apic/apic.c:926
> [<ffffffff82c5c985>] apic_timer_interrupt+0xa5/0xb0 arch/x86/entry/entry_64.S:735
> <EOI> [<ffffffff81188096>] ? native_safe_halt+0x6/0x10 arch/x86/include/asm/irqflags.h:52
> [<ffffffff810834d7>] arch_safe_halt arch/x86/include/asm/paravirt.h:111 [inline]
> [<ffffffff810834d7>] default_idle+0x27/0x430 arch/x86/kernel/process.c:446
> [<ffffffff81085f05>] arch_cpu_idle+0x15/0x20 arch/x86/kernel/process.c:437
> [<ffffffff8132abc3>] default_idle_call+0x53/0x90 kernel/sched/idle.c:92
> [<ffffffff8132b32d>] cpuidle_idle_call kernel/sched/idle.c:156 [inline]
> [<ffffffff8132b32d>] cpu_idle_loop kernel/sched/idle.c:251 [inline]
> [<ffffffff8132b32d>] cpu_startup_entry+0x60d/0x9a0 kernel/sched/idle.c:299
> [<ffffffff8113e119>] start_secondary+0x3c9/0x560 arch/x86/kernel/smpboot.c:245
>
> xfrm_add_policy calls verify_newpolicy_info to check user's policy info,
> but it doest check policy index correcly, if the policy index(ex. 6) is great
> than XFRM_POLICY_MAX which is 3 for now, the validation check will be bypassed.
> It will trigger out of bounds access.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: e682adf021be ("xfrm: Try to honor policy index if it's supplied by user")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> net/xfrm/xfrm_user.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index a131f9f..c2de950 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1424,7 +1424,8 @@ static int verify_newpolicy_info(struct xfrm_userpolicy_info *p)
> ret = verify_policy_dir(p->dir);
> if (ret)
> return ret;
> - if (p->index && ((p->index & XFRM_POLICY_MAX) != p->dir))
> + if (p->index && (p->index > XFRM_POLICY_MAX &&
> + (p->index & XFRM_POLICY_MAX) != p->dir))
> return -EINVAL;
>
> return 0;
>
^ permalink raw reply
* [PATCH net-next] tcp: remove unused parameter of tcp_sacktag_bsearch()
From: Taehee Yoo @ 2019-02-25 9:42 UTC (permalink / raw)
To: davem, edumazet, netdev; +Cc: Taehee Yoo
parameter state in the tcp_sacktag_bsearch() is not used.
So, it can be removed.
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
net/ipv4/tcp_input.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7a027dec649b..6ac274249961 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1574,9 +1574,7 @@ static struct sk_buff *tcp_sacktag_walk(struct sk_buff *skb, struct sock *sk,
return skb;
}
-static struct sk_buff *tcp_sacktag_bsearch(struct sock *sk,
- struct tcp_sacktag_state *state,
- u32 seq)
+static struct sk_buff *tcp_sacktag_bsearch(struct sock *sk, u32 seq)
{
struct rb_node *parent, **p = &sk->tcp_rtx_queue.rb_node;
struct sk_buff *skb;
@@ -1598,13 +1596,12 @@ static struct sk_buff *tcp_sacktag_bsearch(struct sock *sk,
}
static struct sk_buff *tcp_sacktag_skip(struct sk_buff *skb, struct sock *sk,
- struct tcp_sacktag_state *state,
u32 skip_to_seq)
{
if (skb && after(TCP_SKB_CB(skb)->seq, skip_to_seq))
return skb;
- return tcp_sacktag_bsearch(sk, state, skip_to_seq);
+ return tcp_sacktag_bsearch(sk, skip_to_seq);
}
static struct sk_buff *tcp_maybe_skipping_dsack(struct sk_buff *skb,
@@ -1617,7 +1614,7 @@ static struct sk_buff *tcp_maybe_skipping_dsack(struct sk_buff *skb,
return skb;
if (before(next_dup->start_seq, skip_to_seq)) {
- skb = tcp_sacktag_skip(skb, sk, state, next_dup->start_seq);
+ skb = tcp_sacktag_skip(skb, sk, next_dup->start_seq);
skb = tcp_sacktag_walk(skb, sk, NULL, state,
next_dup->start_seq, next_dup->end_seq,
1);
@@ -1758,8 +1755,7 @@ tcp_sacktag_write_queue(struct sock *sk, const struct sk_buff *ack_skb,
/* Head todo? */
if (before(start_seq, cache->start_seq)) {
- skb = tcp_sacktag_skip(skb, sk, state,
- start_seq);
+ skb = tcp_sacktag_skip(skb, sk, start_seq);
skb = tcp_sacktag_walk(skb, sk, next_dup,
state,
start_seq,
@@ -1785,7 +1781,7 @@ tcp_sacktag_write_queue(struct sock *sk, const struct sk_buff *ack_skb,
goto walk;
}
- skb = tcp_sacktag_skip(skb, sk, state, cache->end_seq);
+ skb = tcp_sacktag_skip(skb, sk, cache->end_seq);
/* Check overlap against next cached too (past this one already) */
cache++;
continue;
@@ -1796,7 +1792,7 @@ tcp_sacktag_write_queue(struct sock *sk, const struct sk_buff *ack_skb,
if (!skb)
break;
}
- skb = tcp_sacktag_skip(skb, sk, state, start_seq);
+ skb = tcp_sacktag_skip(skb, sk, start_seq);
walk:
skb = tcp_sacktag_walk(skb, sk, next_dup, state,
--
2.17.1
^ permalink raw reply related
* RE: [PATCH net] staging: fsl-dpaa2: ethsw: Add missing netdevice check
From: Ioana Ciornei @ 2019-02-25 9:43 UTC (permalink / raw)
To: Florian Fainelli, netdev@vger.kernel.org
Cc: Ioana Ciocoi Radulescu, Greg Kroah-Hartman,
open list:DPAA2 ETHERNET SWITCH DRIVER,
open list:STAGING SUBSYSTEM
In-Reply-To: <c9bd0756-2824-a2c2-24ad-a69dc41d9ac3@gmail.com>
> Subject: Re: [PATCH net] staging: fsl-dpaa2: ethsw: Add missing netdevice check
>
> Le 2/23/19 à 12:45 AM, Ioana Ciornei a écrit :
> >
> >> Subject: [PATCH net] staging: fsl-dpaa2: ethsw: Add missing netdevice
> >> check
> >>
> >> port_switchdev_event() does not check that the target network device
> >> is actually backed by the ethsw driver, this could be problematic in
> >> a stacked environment case.
> >>
> >
> > Just FYI, I sent a patch set containing a similar patch verifying if the netdev is
> backed by the ethsw:
> > https://lkml.org/lkml/2019/2/6/216
> >
> > I sent the entire patch set against the staging tree.
>
> Thank you. BTW do you have any plans for moving this driver out of staging? It
> looks pretty good to me to get promoted in tree, provided that you also have all
> the dependencies in place etc.
Driver dependencies such as the fsl-mc bus or the dpio driver are out of staging but not all the TODO items are yet implemented (most notably support for control traffic).
Do you think maybe we can move it now and submit directly to the netdev tree the other features?
Ioana C
^ permalink raw reply
* Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification
From: Ido Schimmel @ 2019-02-25 9:49 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev@vger.kernel.org, David S. Miller, open list,
open list:STAGING SUBSYSTEM, moderated list:ETHERNET BRIDGE,
Jiri Pirko, andrew@lunn.ch, vivien.didelot@gmail.com
In-Reply-To: <daaa3bb6-78b1-7c19-e2fd-6ec03d56201d@gmail.com>
On Sun, Feb 24, 2019 at 08:47:27AM -0800, Florian Fainelli wrote:
> Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit :
> > On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote:
> >> - if (attr->flags & SWITCHDEV_F_NO_RECURSE)
> >> + if (attr & SWITCHDEV_F_DEFER)
> >> + rc = call_switchdev_blocking_notifiers(nt, dev,
> >> + &attr_info.info, NULL);
> >> + else
> >> + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL);
> >
> > I don't believe this is needed. You're calling this function from
> > switchdev_port_attr_set_now() which is always called from process
> > context. switchdev_port_attr_set() takes care of that. Similar to
> > switchdev_port_obj_add().
>
> Except for net/bridge/br_switchdev.c when we check the bridge port's
> flags support with PRE_BRIDGE_FLAGS. In that case we are executing from
> the caller (atomic) context and we can't defer otherwise that trumps the
> whole idea of being able to do a quick check and return that to the
> caller that we cannot support specific flags. How would you recommend
> approaching that?
In this case you can invoke call_switchdev_notifiers() directly from
br_switchdev_set_port_flag(). Eventually switchdev_port_attr_set() will
be gone and bridge code will invoke the notifiers directly.
^ permalink raw reply
* Re: [PATCH] ath9k: remove set but not used variable 'acq'
From: Toke Høiland-Jørgensen @ 2019-02-25 9:50 UTC (permalink / raw)
To: YueHaibing, QCA ath9k Development, Kalle Valo
Cc: YueHaibing, linux-wireless, netdev, kernel-janitors
In-Reply-To: <20190225033246.127410-1-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> writes:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/wireless/ath/ath9k/recv.c: In function 'ath_rx_count_airtime':
> drivers/net/wireless/ath/ath9k/recv.c:1010:18: warning:
> variable 'acq' set but not used [-Wunused-but-set-variable]
>
> It's not used after 89cea7493a34 ("ath9k: Switch to mac80211 TXQ scheduling
> and airtime APIs"). Also remove related variables.
Ah, right, seems I forgot to clean that up. I wonder why I didn't get a
compiler warning for it. Anyway, nice catch :)
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
-Toke
^ permalink raw reply
* [PATCH v2] xfrm: correctly check policy index in verify_newpolicy_info
From: Yue Haibing @ 2019-02-25 9:56 UTC (permalink / raw)
To: steffen.klassert, herbert, davem; +Cc: linux-kernel, netdev, YueHaibing
From: YueHaibing <yuehaibing@huawei.com>
UBSAN report this:
UBSAN: Undefined behaviour in net/xfrm/xfrm_policy.c:1289:24
index 6 is out of range for type 'unsigned int [6]'
CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.4.162-514.55.6.9.x86_64+ #13
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
0000000000000000 1466cf39b41b23c9 ffff8801f6b07a58 ffffffff81cb35f4
0000000041b58ab3 ffffffff83230f9c ffffffff81cb34e0 ffff8801f6b07a80
ffff8801f6b07a20 1466cf39b41b23c9 ffffffff851706e0 ffff8801f6b07ae8
Call Trace:
<IRQ> [<ffffffff81cb35f4>] __dump_stack lib/dump_stack.c:15 [inline]
<IRQ> [<ffffffff81cb35f4>] dump_stack+0x114/0x1a0 lib/dump_stack.c:51
[<ffffffff81d94225>] ubsan_epilogue+0x12/0x8f lib/ubsan.c:164
[<ffffffff81d954db>] __ubsan_handle_out_of_bounds+0x16e/0x1b2 lib/ubsan.c:382
[<ffffffff82a25acd>] __xfrm_policy_unlink+0x3dd/0x5b0 net/xfrm/xfrm_policy.c:1289
[<ffffffff82a2e572>] xfrm_policy_delete+0x52/0xb0 net/xfrm/xfrm_policy.c:1309
[<ffffffff82a3319b>] xfrm_policy_timer+0x30b/0x590 net/xfrm/xfrm_policy.c:243
[<ffffffff813d3927>] call_timer_fn+0x237/0x990 kernel/time/timer.c:1144
[<ffffffff813d8e7e>] __run_timers kernel/time/timer.c:1218 [inline]
[<ffffffff813d8e7e>] run_timer_softirq+0x6ce/0xb80 kernel/time/timer.c:1401
[<ffffffff8120d6f9>] __do_softirq+0x299/0xe10 kernel/softirq.c:273
[<ffffffff8120e676>] invoke_softirq kernel/softirq.c:350 [inline]
[<ffffffff8120e676>] irq_exit+0x216/0x2c0 kernel/softirq.c:391
[<ffffffff82c5edab>] exiting_irq arch/x86/include/asm/apic.h:652 [inline]
[<ffffffff82c5edab>] smp_apic_timer_interrupt+0x8b/0xc0 arch/x86/kernel/apic/apic.c:926
[<ffffffff82c5c985>] apic_timer_interrupt+0xa5/0xb0 arch/x86/entry/entry_64.S:735
<EOI> [<ffffffff81188096>] ? native_safe_halt+0x6/0x10 arch/x86/include/asm/irqflags.h:52
[<ffffffff810834d7>] arch_safe_halt arch/x86/include/asm/paravirt.h:111 [inline]
[<ffffffff810834d7>] default_idle+0x27/0x430 arch/x86/kernel/process.c:446
[<ffffffff81085f05>] arch_cpu_idle+0x15/0x20 arch/x86/kernel/process.c:437
[<ffffffff8132abc3>] default_idle_call+0x53/0x90 kernel/sched/idle.c:92
[<ffffffff8132b32d>] cpuidle_idle_call kernel/sched/idle.c:156 [inline]
[<ffffffff8132b32d>] cpu_idle_loop kernel/sched/idle.c:251 [inline]
[<ffffffff8132b32d>] cpu_startup_entry+0x60d/0x9a0 kernel/sched/idle.c:299
[<ffffffff8113e119>] start_secondary+0x3c9/0x560 arch/x86/kernel/smpboot.c:245
xfrm_add_policy calls verify_newpolicy_info to check user's policy info,
but it does not check policy index correcly, if the policy index(ex. 6) is great
than or equal to the twice of XFRM_POLICY_MAX (XFRM_POLICY_MAX = 3), it may pass
the check. Then __xfrm_policy_unlink use the index to access array policy_count
whose size is XFRM_POLICY_MAX * 2, triggering out of bounds access.
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: e682adf021be ("xfrm: Try to honor policy index if it's supplied by user")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
v2: respin the patch
---
net/xfrm/xfrm_user.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index a131f9f..60adacf 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1424,7 +1424,8 @@ static int verify_newpolicy_info(struct xfrm_userpolicy_info *p)
ret = verify_policy_dir(p->dir);
if (ret)
return ret;
- if (p->index && ((p->index & XFRM_POLICY_MAX) != p->dir))
+ if (p->index && ((p->index >= XFRM_POLICY_MAX * 2) ||
+ (p->index & XFRM_POLICY_MAX) != p->dir))
return -EINVAL;
return 0;
--
2.7.0
^ permalink raw reply related
* Re: [PATCH] ath9k: remove set but not used variable 'acq'
From: Kalle Valo @ 2019-02-25 10:03 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: YueHaibing, QCA ath9k Development, linux-wireless, netdev,
kernel-janitors
In-Reply-To: <87lg24urgq.fsf@toke.dk>
Toke Høiland-Jørgensen <toke@redhat.com> writes:
> YueHaibing <yuehaibing@huawei.com> writes:
>
>> Fixes gcc '-Wunused-but-set-variable' warning:
>>
>> drivers/net/wireless/ath/ath9k/recv.c: In function 'ath_rx_count_airtime':
>> drivers/net/wireless/ath/ath9k/recv.c:1010:18: warning:
>> variable 'acq' set but not used [-Wunused-but-set-variable]
>>
>> It's not used after 89cea7493a34 ("ath9k: Switch to mac80211 TXQ scheduling
>> and airtime APIs"). Also remove related variables.
>
> Ah, right, seems I forgot to clean that up. I wonder why I didn't get a
> compiler warning for it.
I think the warning is not enabled by default and you need to use W=1
Makefile variable to enable it.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH v2 bpf-next 4/9] bpf: add bpf helper bpf_skb_ecn_set_ce
From: Daniel Borkmann @ 2019-02-25 10:10 UTC (permalink / raw)
To: Martin Lau
Cc: Lawrence Brakmo, netdev, Alexei Starovoitov, Eric Dumazet,
Kernel Team
In-Reply-To: <20190223073031.utnow4seviqyfqta@kafai-mbp.dhcp.thefacebook.com>
On 02/23/2019 08:30 AM, Martin Lau wrote:
> On Sat, Feb 23, 2019 at 02:14:26AM +0100, Daniel Borkmann wrote:
>> On 02/23/2019 02:06 AM, brakmo wrote:
>>> This patch adds a new bpf helper BPF_FUNC_skb_ecn_set_ce
>>> "int bpf_skb_ecn_set_ce(struct sk_buff *skb)". It is added to
>>> BPF_PROG_TYPE_CGROUP_SKB typed bpf_prog which currently can
>>> be attached to the ingress and egress path. The helper is needed
>>> because his type of bpf_prog cannot modify the skb directly.
>>>
>>> This helper is used to set the ECN field of ECN capable IP packets to ce
>>> (congestion encountered) in the IPv6 or IPv4 header of the skb. It can be
>>> used by a bpf_prog to manage egress or ingress network bandwdith limit
>>> per cgroupv2 by inducing an ECN response in the TCP sender.
>>> This works best when using DCTCP.
>>>
>>> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
>>> ---
>>> include/uapi/linux/bpf.h | 10 +++++++++-
>>> net/core/filter.c | 14 ++++++++++++++
>>> 2 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 95b5058fa945..fc646f3eaf9b 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -2365,6 +2365,13 @@ union bpf_attr {
>>> * Make a tcp_sock enter CWR state.
>>> * Return
>>> * 0 on success, or a negative error in case of failure.
>>> + *
>>> + * int bpf_skb_ecn_set_ce(struct sk_buf *skb)
>>> + * Description
>>> + * Sets ECN of IP header to ce (congestion encountered) if
>>> + * current value is ect (ECN capable). Works with IPv6 and IPv4.
>>> + * Return
>>> + * 1 if set, 0 if not set.
>>> */
>>> #define __BPF_FUNC_MAPPER(FN) \
>>> FN(unspec), \
>>> @@ -2464,7 +2471,8 @@ union bpf_attr {
>>> FN(spin_unlock), \
>>> FN(sk_fullsock), \
>>> FN(tcp_sock), \
>>> - FN(tcp_enter_cwr),
>>> + FN(tcp_enter_cwr), \
>>> + FN(skb_ecn_set_ce),
>>>
>>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>>> * function eBPF program intends to call
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index ca57ef25279c..955369c6ed30 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -5444,6 +5444,18 @@ static const struct bpf_func_proto bpf_tcp_enter_cwr_proto = {
>>> .ret_type = RET_INTEGER,
>>> .arg1_type = ARG_PTR_TO_TCP_SOCK,
>>> };
>>> +
>>> +BPF_CALL_1(bpf_skb_ecn_set_ce, struct sk_buff *, skb)
>>> +{
>>> + return INET_ECN_set_ce(skb);
>>
>> Hm, but as mentioned last time, don't we have to ensure here that skb
>> is writable (aka skb->data private to us before writing into it)?
> INET_ECN_set_ce(skb) is also called from a few net/sched/sch_*.c
> but I don't see how they ensure if a skb is writable.
>
> May be I have missed something there that can also be borrowed and
> reused here?
My understanding is that before doing any writes into skb, we should make
sure the data area is private to us (and offset in linear data). In tc BPF
(ingress, egress) we use bpf_try_make_writable() helper for this, others
like act_{pedit,skbmod} or ovs have similar logic before writing into skb,
note that in all these cases it's mostly about generic writes, so location
could also be L4, for example.
Difference of above helper compared to net/sched/sch_*.c instances could
be that it's i) for the qdisc case it's only on egress INET_ECN_set_ce()
and that there may be a convention that qdiscs specifically may mangle
it whereas the helper could be called on ingress and egress and confuse
other subsystems since they won't see original or race by seeing partially
updated (invalid) packet.
Eric, have a chance to clarify? Perhaps then would make sense to disallow
the helper in cgroup ingress path.
^ permalink raw reply
* [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Leslie Monis @ 2019-02-25 10:20 UTC (permalink / raw)
To: jhs; +Cc: netdev, dave, Leslie Monis
The current implementation of the PIE queuing discipline is according to the
IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and the paper
[PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
However, a lot of necessary modifications and enhancements have been proposed
in RFC 8033, which have not yet been incorporated in the source code of Linux.
This patch series helps in achieving the same.
Performance tests carried out using Flent [https://flent.org/]
Changes from v1 to v2:
- Excluded the patch setting PIE dynamically active/inactive as the test
results were unsatisfactory
- Fixed a scaling issue when adding more auto-tuning cases which caused
local variables to underflow
- Changed the long if/else chain to a loop as suggested by Stephen
- Changed the position of the accu_prob variable in the pie_vars
structure as recommended by Stephen
Mohit P. Tahiliani (7):
net: sched: pie: change value of QUEUE_THRESHOLD
net: sched: pie: change default value of pie_params->target
net: sched: pie: change default value of pie_params->tupdate
net: sched: pie: change initial value of pie_vars->burst_time
net: sched: pie: add more cases to auto-tune alpha and beta
net: sched: pie: add derandomization mechanism
net: sched: pie: update references
include/uapi/linux/pkt_sched.h | 2 +-
net/sched/sch_pie.c | 107 ++++++++++++++++++++-------------
2 files changed, 66 insertions(+), 43 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH net-next v2 1/7] net: sched: pie: change value of QUEUE_THRESHOLD
From: Leslie Monis @ 2019-02-25 10:20 UTC (permalink / raw)
To: jhs
Cc: netdev, dave, Mohit P. Tahiliani, Dhaval Khandla,
Hrishikesh Hiraskar, Manish Kumar B, Sachin D . Patil,
Leslie Monis
In-Reply-To: <20190225102051.12268-1-lesliemonis@gmail.com>
From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in>
RFC 8033 recommends a value of 16384 bytes for the queue
threshold.
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
Signed-off-by: Dhaval Khandla <dhavaljkhandla26@gmail.com>
Signed-off-by: Hrishikesh Hiraskar <hrishihiraskar@gmail.com>
Signed-off-by: Manish Kumar B <bmanish15597@gmail.com>
Signed-off-by: Sachin D. Patil <sdp.sachin@gmail.com>
Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
---
net/sched/sch_pie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index d1429371592f..7778eff6cdb7 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -31,7 +31,7 @@
#include <net/pkt_sched.h>
#include <net/inet_ecn.h>
-#define QUEUE_THRESHOLD 10000
+#define QUEUE_THRESHOLD 16384
#define DQCOUNT_INVALID -1
#define MAX_PROB 0xffffffff
#define PIE_SCALE 8
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v2 2/7] net: sched: pie: change default value of pie_params->target
From: Leslie Monis @ 2019-02-25 10:20 UTC (permalink / raw)
To: jhs
Cc: netdev, dave, Mohit P. Tahiliani, Dhaval Khandla,
Hrishikesh Hiraskar, Manish Kumar B, Sachin D . Patil,
Leslie Monis
In-Reply-To: <20190225102051.12268-1-lesliemonis@gmail.com>
From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in>
RFC 8033 suggests a default value of 15 milliseconds for the
target queue delay.
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
Signed-off-by: Dhaval Khandla <dhavaljkhandla26@gmail.com>
Signed-off-by: Hrishikesh Hiraskar <hrishihiraskar@gmail.com>
Signed-off-by: Manish Kumar B <bmanish15597@gmail.com>
Signed-off-by: Sachin D. Patil <sdp.sachin@gmail.com>
Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
---
net/sched/sch_pie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 7778eff6cdb7..91af9bf19852 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -83,7 +83,7 @@ static void pie_params_init(struct pie_params *params)
params->beta = 20;
params->tupdate = usecs_to_jiffies(30 * USEC_PER_MSEC); /* 30 ms */
params->limit = 1000; /* default of 1000 packets */
- params->target = PSCHED_NS2TICKS(20 * NSEC_PER_MSEC); /* 20 ms */
+ params->target = PSCHED_NS2TICKS(15 * NSEC_PER_MSEC); /* 15 ms */
params->ecn = false;
params->bytemode = false;
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v2 3/7] net: sched: pie: change default value of pie_params->tupdate
From: Leslie Monis @ 2019-02-25 10:20 UTC (permalink / raw)
To: jhs
Cc: netdev, dave, Mohit P. Tahiliani, Dhaval Khandla,
Hrishikesh Hiraskar, Manish Kumar B, Sachin D . Patil,
Leslie Monis
In-Reply-To: <20190225102051.12268-1-lesliemonis@gmail.com>
From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in>
RFC 8033 suggests a default value of 15 milliseconds for the
update interval.
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
Signed-off-by: Dhaval Khandla <dhavaljkhandla26@gmail.com>
Signed-off-by: Hrishikesh Hiraskar <hrishihiraskar@gmail.com>
Signed-off-by: Manish Kumar B <bmanish15597@gmail.com>
Signed-off-by: Sachin D. Patil <sdp.sachin@gmail.com>
Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
---
net/sched/sch_pie.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 91af9bf19852..702f75afc312 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -81,7 +81,7 @@ static void pie_params_init(struct pie_params *params)
{
params->alpha = 2;
params->beta = 20;
- params->tupdate = usecs_to_jiffies(30 * USEC_PER_MSEC); /* 30 ms */
+ params->tupdate = usecs_to_jiffies(15 * USEC_PER_MSEC); /* 15 ms */
params->limit = 1000; /* default of 1000 packets */
params->target = PSCHED_NS2TICKS(15 * NSEC_PER_MSEC); /* 15 ms */
params->ecn = false;
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v2 4/7] net: sched: pie: change initial value of pie_vars->burst_time
From: Leslie Monis @ 2019-02-25 10:20 UTC (permalink / raw)
To: jhs
Cc: netdev, dave, Mohit P. Tahiliani, Dhaval Khandla,
Hrishikesh Hiraskar, Manish Kumar B, Sachin D . Patil,
Leslie Monis
In-Reply-To: <20190225102051.12268-1-lesliemonis@gmail.com>
From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in>
RFC 8033 suggests an initial value of 150 milliseconds for
the maximum time allowed for a burst of packets.
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
Signed-off-by: Dhaval Khandla <dhavaljkhandla26@gmail.com>
Signed-off-by: Hrishikesh Hiraskar <hrishihiraskar@gmail.com>
Signed-off-by: Manish Kumar B <bmanish15597@gmail.com>
Signed-off-by: Sachin D. Patil <sdp.sachin@gmail.com>
Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
---
net/sched/sch_pie.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 702f75afc312..d88ab53593b3 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -92,8 +92,8 @@ static void pie_vars_init(struct pie_vars *vars)
{
vars->dq_count = DQCOUNT_INVALID;
vars->avg_dq_rate = 0;
- /* default of 100 ms in pschedtime */
- vars->burst_time = PSCHED_NS2TICKS(100 * NSEC_PER_MSEC);
+ /* default of 150 ms in pschedtime */
+ vars->burst_time = PSCHED_NS2TICKS(150 * NSEC_PER_MSEC);
}
static bool drop_early(struct Qdisc *sch, u32 packet_size)
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v2 5/7] net: sched: pie: add more cases to auto-tune alpha and beta
From: Leslie Monis @ 2019-02-25 10:20 UTC (permalink / raw)
To: jhs
Cc: netdev, dave, Mohit P. Tahiliani, Dhaval Khandla,
Hrishikesh Hiraskar, Manish Kumar B, Sachin D . Patil,
Leslie Monis
In-Reply-To: <20190225102051.12268-1-lesliemonis@gmail.com>
From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in>
The current implementation scales the local alpha and beta
variables in the calculate_probability function by the same
amount for all values of drop probability below 1%.
RFC 8033 suggests using additional cases for auto-tuning
alpha and beta when the drop probability is less than 1%.
In order to add more auto-tuning cases, MAX_PROB must be
scaled by u64 instead of u32 to prevent underflow when
scaling the local alpha and beta variables in the
calculate_probability function.
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
Signed-off-by: Dhaval Khandla <dhavaljkhandla26@gmail.com>
Signed-off-by: Hrishikesh Hiraskar <hrishihiraskar@gmail.com>
Signed-off-by: Manish Kumar B <bmanish15597@gmail.com>
Signed-off-by: Sachin D. Patil <sdp.sachin@gmail.com>
Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
---
include/uapi/linux/pkt_sched.h | 2 +-
net/sched/sch_pie.c | 65 +++++++++++++++++-----------------
2 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 0d18b1d1fbbc..1eb572ef3f27 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -954,7 +954,7 @@ enum {
#define TCA_PIE_MAX (__TCA_PIE_MAX - 1)
struct tc_pie_xstats {
- __u32 prob; /* current probability */
+ __u64 prob; /* current probability */
__u32 delay; /* current delay in ms */
__u32 avg_dq_rate; /* current average dq_rate in bits/pie_time */
__u32 packets_in; /* total number of packets enqueued */
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index d88ab53593b3..b9f586cfb51c 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -33,7 +33,7 @@
#define QUEUE_THRESHOLD 16384
#define DQCOUNT_INVALID -1
-#define MAX_PROB 0xffffffff
+#define MAX_PROB 0xffffffffffffffff
#define PIE_SCALE 8
/* parameters used */
@@ -49,7 +49,7 @@ struct pie_params {
/* variables used */
struct pie_vars {
- u32 prob; /* probability but scaled by u32 limit. */
+ u64 prob; /* probability but scaled by u64 limit. */
psched_time_t burst_time;
psched_time_t qdelay;
psched_time_t qdelay_old;
@@ -99,8 +99,8 @@ static void pie_vars_init(struct pie_vars *vars)
static bool drop_early(struct Qdisc *sch, u32 packet_size)
{
struct pie_sched_data *q = qdisc_priv(sch);
- u32 rnd;
- u32 local_prob = q->vars.prob;
+ u64 rnd;
+ u64 local_prob = q->vars.prob;
u32 mtu = psched_mtu(qdisc_dev(sch));
/* If there is still burst allowance left skip random early drop */
@@ -124,11 +124,11 @@ static bool drop_early(struct Qdisc *sch, u32 packet_size)
* probablity. Smaller packets will have lower drop prob in this case
*/
if (q->params.bytemode && packet_size <= mtu)
- local_prob = (local_prob / mtu) * packet_size;
+ local_prob = (local_prob / (u64)mtu) * (u64)packet_size;
else
local_prob = q->vars.prob;
- rnd = prandom_u32();
+ prandom_bytes(&rnd, 8);
if (rnd < local_prob)
return true;
@@ -317,9 +317,10 @@ static void calculate_probability(struct Qdisc *sch)
u32 qlen = sch->qstats.backlog; /* queue size in bytes */
psched_time_t qdelay = 0; /* in pschedtime */
psched_time_t qdelay_old = q->vars.qdelay; /* in pschedtime */
- s32 delta = 0; /* determines the change in probability */
- u32 oldprob;
- u32 alpha, beta;
+ s64 delta = 0; /* determines the change in probability */
+ u64 oldprob;
+ u64 alpha, beta;
+ u32 power;
bool update_prob = true;
q->vars.qdelay_old = q->vars.qdelay;
@@ -339,38 +340,36 @@ static void calculate_probability(struct Qdisc *sch)
* value for alpha as 0.125. In this implementation, we use values 0-32
* passed from user space to represent this. Also, alpha and beta have
* unit of HZ and need to be scaled before they can used to update
- * probability. alpha/beta are updated locally below by 1) scaling them
- * appropriately 2) scaling down by 16 to come to 0-2 range.
- * Please see paper for details.
- *
- * We scale alpha and beta differently depending on whether we are in
- * light, medium or high dropping mode.
+ * probability. alpha/beta are updated locally below by scaling down
+ * by 16 to come to 0-2 range.
*/
- if (q->vars.prob < MAX_PROB / 100) {
- alpha =
- (q->params.alpha * (MAX_PROB / PSCHED_TICKS_PER_SEC)) >> 7;
- beta =
- (q->params.beta * (MAX_PROB / PSCHED_TICKS_PER_SEC)) >> 7;
- } else if (q->vars.prob < MAX_PROB / 10) {
- alpha =
- (q->params.alpha * (MAX_PROB / PSCHED_TICKS_PER_SEC)) >> 5;
- beta =
- (q->params.beta * (MAX_PROB / PSCHED_TICKS_PER_SEC)) >> 5;
- } else {
- alpha =
- (q->params.alpha * (MAX_PROB / PSCHED_TICKS_PER_SEC)) >> 4;
- beta =
- (q->params.beta * (MAX_PROB / PSCHED_TICKS_PER_SEC)) >> 4;
+ alpha = ((u64)q->params.alpha * (MAX_PROB / PSCHED_TICKS_PER_SEC)) >> 4;
+ beta = ((u64)q->params.beta * (MAX_PROB / PSCHED_TICKS_PER_SEC)) >> 4;
+
+ /* We scale alpha and beta differently depending on how heavy the
+ * congestion is. Please see RFC 8033 for details.
+ */
+ if (q->vars.prob < MAX_PROB / 10) {
+ alpha >>= 1;
+ beta >>= 1;
+
+ power = 100;
+ while (q->vars.prob < MAX_PROB / (u64)power &&
+ power <= 1000000) {
+ alpha >>= 2;
+ beta >>= 2;
+ power *= 10;
+ }
}
/* alpha and beta should be between 0 and 32, in multiples of 1/16 */
- delta += alpha * ((qdelay - q->params.target));
- delta += beta * ((qdelay - qdelay_old));
+ delta += alpha * (u64)(qdelay - q->params.target);
+ delta += beta * (u64)(qdelay - qdelay_old);
oldprob = q->vars.prob;
/* to ensure we increase probability in steps of no more than 2% */
- if (delta > (s32)(MAX_PROB / (100 / 2)) &&
+ if (delta > (s64)(MAX_PROB / (100 / 2)) &&
q->vars.prob >= MAX_PROB / 10)
delta = (MAX_PROB / 100) * 2;
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v2 6/7] net: sched: pie: add derandomization mechanism
From: Leslie Monis @ 2019-02-25 10:20 UTC (permalink / raw)
To: jhs
Cc: netdev, dave, Mohit P. Tahiliani, Dhaval Khandla,
Hrishikesh Hiraskar, Manish Kumar B, Sachin D . Patil,
Leslie Monis
In-Reply-To: <20190225102051.12268-1-lesliemonis@gmail.com>
From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in>
Random dropping of packets to achieve latency control may
introduce outlier situations where packets are dropped too
close to each other or too far from each other. This can
cause the real drop percentage to temporarily deviate from
the intended drop probability. In certain scenarios, such
as a small number of simultaneous TCP flows, these
deviations can cause significant deviations in link
utilization and queuing latency.
RFC 8033 suggests using a derandomization mechanism to avoid
these deviations.
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
Signed-off-by: Dhaval Khandla <dhavaljkhandla26@gmail.com>
Signed-off-by: Hrishikesh Hiraskar <hrishihiraskar@gmail.com>
Signed-off-by: Manish Kumar B <bmanish15597@gmail.com>
Signed-off-by: Sachin D. Patil <sdp.sachin@gmail.com>
Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
---
net/sched/sch_pie.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index b9f586cfb51c..8bfd8ca500f9 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -55,8 +55,10 @@ struct pie_vars {
psched_time_t qdelay_old;
u64 dq_count; /* measured in bytes */
psched_time_t dq_tstamp; /* drain rate */
+ u64 accu_prob; /* accumulated drop probability */
u32 avg_dq_rate; /* bytes per pschedtime tick,scaled */
u32 qlen_old; /* in bytes */
+ u8 accu_prob_overflows; /* overflows of accu_prob */
};
/* statistics gathering */
@@ -91,9 +93,11 @@ static void pie_params_init(struct pie_params *params)
static void pie_vars_init(struct pie_vars *vars)
{
vars->dq_count = DQCOUNT_INVALID;
+ vars->accu_prob = 0;
vars->avg_dq_rate = 0;
/* default of 150 ms in pschedtime */
vars->burst_time = PSCHED_NS2TICKS(150 * NSEC_PER_MSEC);
+ vars->accu_prob_overflows = 0;
}
static bool drop_early(struct Qdisc *sch, u32 packet_size)
@@ -128,9 +132,29 @@ static bool drop_early(struct Qdisc *sch, u32 packet_size)
else
local_prob = q->vars.prob;
+ if (local_prob == 0) {
+ q->vars.accu_prob = 0;
+ q->vars.accu_prob_overflows = 0;
+ }
+
+ if (local_prob > MAX_PROB - q->vars.accu_prob)
+ q->vars.accu_prob_overflows++;
+
+ q->vars.accu_prob += local_prob;
+
+ if (q->vars.accu_prob_overflows == 0 &&
+ q->vars.accu_prob < (MAX_PROB / 100) * 85)
+ return false;
+ if (q->vars.accu_prob_overflows == 8 &&
+ q->vars.accu_prob >= MAX_PROB / 2)
+ return true;
+
prandom_bytes(&rnd, 8);
- if (rnd < local_prob)
+ if (rnd < local_prob) {
+ q->vars.accu_prob = 0;
+ q->vars.accu_prob_overflows = 0;
return true;
+ }
return false;
}
@@ -168,6 +192,8 @@ static int pie_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
out:
q->stats.dropped++;
+ q->vars.accu_prob = 0;
+ q->vars.accu_prob_overflows = 0;
return qdisc_drop(skb, sch, to_free);
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v2 7/7] net: sched: pie: update references
From: Leslie Monis @ 2019-02-25 10:20 UTC (permalink / raw)
To: jhs
Cc: netdev, dave, Mohit P. Tahiliani, Dhaval Khandla,
Hrishikesh Hiraskar, Manish Kumar B, Sachin D . Patil,
Leslie Monis
In-Reply-To: <20190225102051.12268-1-lesliemonis@gmail.com>
From: "Mohit P. Tahiliani" <tahiliani@nitk.edu.in>
RFC 8033 replaces the IETF draft for PIE
Signed-off-by: Mohit P. Tahiliani <tahiliani@nitk.edu.in>
Signed-off-by: Dhaval Khandla <dhavaljkhandla26@gmail.com>
Signed-off-by: Hrishikesh Hiraskar <hrishihiraskar@gmail.com>
Signed-off-by: Manish Kumar B <bmanish15597@gmail.com>
Signed-off-by: Sachin D. Patil <sdp.sachin@gmail.com>
Signed-off-by: Leslie Monis <lesliemonis@gmail.com>
---
net/sched/sch_pie.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 8bfd8ca500f9..78658080fcc4 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -17,9 +17,7 @@
* University of Oslo, Norway.
*
* References:
- * IETF draft submission: http://tools.ietf.org/html/draft-pan-aqm-pie-00
- * IEEE Conference on High Performance Switching and Routing 2013 :
- * "PIE: A * Lightweight Control Scheme to Address the Bufferbloat Problem"
+ * RFC 8033: https://tools.ietf.org/html/rfc8034
*/
#include <linux/module.h>
--
2.17.1
^ permalink raw reply related
* [PATCH] tcp: clean up SOCK_DEBUG()
From: Yafang Shao @ 2019-02-25 10:33 UTC (permalink / raw)
To: davem; +Cc: netdev, shaoyafang, Yafang Shao
Per discussion with Daniel[1] and Eric[2], these SOCK_DEBUG() calles in
TCP are not needed now.
We'd better clean up it.
[1] https://patchwork.ozlabs.org/patch/1035573/
[2] https://patchwork.ozlabs.org/patch/1040533/
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
net/ipv4/tcp_input.c | 19 +------------------
net/ipv6/tcp_ipv6.c | 2 --
2 files changed, 1 insertion(+), 20 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7a027dec..6d2750e 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3595,7 +3595,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
* this segment (RFC793 Section 3.9).
*/
if (after(ack, tp->snd_nxt))
- goto invalid_ack;
+ return -1;
if (after(ack, prior_snd_una)) {
flag |= FLAG_SND_UNA_ADVANCED;
@@ -3714,10 +3714,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
tcp_process_tlp_ack(sk, ack, flag);
return 1;
-invalid_ack:
- SOCK_DEBUG(sk, "Ack %u after %u:%u\n", ack, tp->snd_una, tp->snd_nxt);
- return -1;
-
old_ack:
/* If data was SACKed, tag it and see if we should send more data.
* If data was DSACKed, see if we can undo a cwnd reduction.
@@ -3731,7 +3727,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
tcp_xmit_recovery(sk, rexmit);
}
- SOCK_DEBUG(sk, "Ack %u before %u:%u\n", ack, tp->snd_una, tp->snd_nxt);
return 0;
}
@@ -4432,13 +4427,9 @@ static void tcp_ofo_queue(struct sock *sk)
rb_erase(&skb->rbnode, &tp->out_of_order_queue);
if (unlikely(!after(TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt))) {
- SOCK_DEBUG(sk, "ofo packet was already received\n");
tcp_drop(sk, skb);
continue;
}
- SOCK_DEBUG(sk, "ofo requeuing : rcv_next %X seq %X - %X\n",
- tp->rcv_nxt, TCP_SKB_CB(skb)->seq,
- TCP_SKB_CB(skb)->end_seq);
tail = skb_peek_tail(&sk->sk_receive_queue);
eaten = tail && tcp_try_coalesce(sk, tail, skb, &fragstolen);
@@ -4502,8 +4493,6 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFOQUEUE);
seq = TCP_SKB_CB(skb)->seq;
end_seq = TCP_SKB_CB(skb)->end_seq;
- SOCK_DEBUG(sk, "out of order segment: rcv_next %X seq %X - %X\n",
- tp->rcv_nxt, seq, end_seq);
p = &tp->out_of_order_queue.rb_node;
if (RB_EMPTY_ROOT(&tp->out_of_order_queue)) {
@@ -4779,10 +4768,6 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
if (before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) {
/* Partial packet, seq < rcv_next < end_seq */
- SOCK_DEBUG(sk, "partial packet: rcv_next %X seq %X - %X\n",
- tp->rcv_nxt, TCP_SKB_CB(skb)->seq,
- TCP_SKB_CB(skb)->end_seq);
-
tcp_dsack_set(sk, TCP_SKB_CB(skb)->seq, tp->rcv_nxt);
/* If window is closed, drop tail of packet. But after
@@ -5061,8 +5046,6 @@ static int tcp_prune_queue(struct sock *sk)
{
struct tcp_sock *tp = tcp_sk(sk);
- SOCK_DEBUG(sk, "prune_queue: c=%x\n", tp->copied_seq);
-
NET_INC_STATS(sock_net(sk), LINUX_MIB_PRUNECALLED);
if (atomic_read(&sk->sk_rmem_alloc) >= sk->sk_rcvbuf)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index e51cda7..57ef69a1 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -220,8 +220,6 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
u32 exthdrlen = icsk->icsk_ext_hdr_len;
struct sockaddr_in sin;
- SOCK_DEBUG(sk, "connect: ipv4 mapped\n");
-
if (__ipv6_only_sock(sk))
return -ENETUNREACH;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH bpf-next 1/4] bpf: enable program stats
From: Daniel Borkmann @ 2019-02-25 10:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Roman Gushchin, Alexei Starovoitov, davem@davemloft.net,
netdev@vger.kernel.org, bpf@vger.kernel.org, Kernel Team
In-Reply-To: <20190223023810.5s7zyj4zqe6poigg@ast-mbp.dhcp.thefacebook.com>
On 02/23/2019 03:38 AM, Alexei Starovoitov wrote:
> On Sat, Feb 23, 2019 at 02:06:56AM +0100, Daniel Borkmann wrote:
>>
>> In general, having some stats and timing info would be useful, but I
>> guess people might want to customize it in future even more specifically
>> beyond number of runs + time it takes. One thing that would be super
>> useful is to have some notion of __attribute__((constructor)) and
>> __attribute__((destructor)) support in BPF which gets then inlined into
>> prologue / epilogue of program. E.g. such infrastructure would allow to
>> mark an skb and measure time it takes through the BPF prog till it hits
>> an exit point somewhere (without having to explicitly code this into the
>> program everywhere). Other examples may be histograms or p99 latencies
>> that might probably be useful. Feels like for monitoring more ways to
>> program would be nice and to move it into the BPF insns sequence (e.g.
>> enforced globally or by choice of prog as another option)? Thoughts?
>
> the constructor/destructor you mean to capture the full sequence of tail_calls?
> Or full path of skb through the stack with all hook points?
> That is likely very useful without any bpf, but I think hw timestamping
> already serves that purpose.
Not through the stack, but was more thinking something like low-overhead
kprobes-style extension for a BPF prog where such sequence would be added
'inline' at beginning / exit of BPF prog invocation with normal ctx access
and helpers as the native program (e.g. time-stamping plus read/write into
mark as one example which kprobes couldn't do). But might go much beyond
context of this stats collection.
> I've been thinking about doing this stats per program
> (instead of static_key for all).
> Like replacing a bpf_prog->bpf_func with a wrapper function
> that does stats, but that is more costly in retpoline world
> due to extra indirect call.
That's actually an interesting thought, given the original prog->bpf_func
is a known address at that time, this could be templated where an inner
dummy bpf_func call to the normal BPF prog gets later replaced with the
actual prog->bpf_func address to have no indirect call. This would also
allow to keep the actual prog->bpf_func entry call-sites small and simple.
> Another alternative is to patch the stats in via JITs, but
> it's much more complex and error prone.
> So went with the simplest approach.
Pure BPF insns rewrite via JIT would indeed be unclear wrt per-cpu data
structures.
> With all stats ideas we need to be careful not to reintroduce what
> perf is already great at.
Yeah agree.
> These stats are _not_ for performance analysis. That's what perf does.
> These stats are for 24/7 monitoring to catch things that not suppose to happen.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH] ath9k: remove set but not used variable 'acq'
From: Toke Høiland-Jørgensen @ 2019-02-25 10:49 UTC (permalink / raw)
To: Kalle Valo
Cc: YueHaibing, QCA ath9k Development, linux-wireless, netdev,
kernel-janitors
In-Reply-To: <87ftscmbh7.fsf@kamboji.qca.qualcomm.com>
Kalle Valo <kvalo@codeaurora.org> writes:
> Toke Høiland-Jørgensen <toke@redhat.com> writes:
>
>> YueHaibing <yuehaibing@huawei.com> writes:
>>
>>> Fixes gcc '-Wunused-but-set-variable' warning:
>>>
>>> drivers/net/wireless/ath/ath9k/recv.c: In function 'ath_rx_count_airtime':
>>> drivers/net/wireless/ath/ath9k/recv.c:1010:18: warning:
>>> variable 'acq' set but not used [-Wunused-but-set-variable]
>>>
>>> It's not used after 89cea7493a34 ("ath9k: Switch to mac80211 TXQ scheduling
>>> and airtime APIs"). Also remove related variables.
>>
>> Ah, right, seems I forgot to clean that up. I wonder why I didn't get a
>> compiler warning for it.
>
> I think the warning is not enabled by default and you need to use W=1
> Makefile variable to enable it.
Hmm, right, thanks! Guess I should get into the habit of compiling with
warnings enabled before submitting patches :)
-Toke
^ permalink raw reply
* Re: [PATCH] ath9k: remove set but not used variable 'acq'
From: Kalle Valo @ 2019-02-25 10:59 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: YueHaibing, QCA ath9k Development, linux-wireless, netdev,
kernel-janitors
In-Reply-To: <87imx8uor2.fsf@toke.dk>
Toke Høiland-Jørgensen <toke@redhat.com> writes:
> Kalle Valo <kvalo@codeaurora.org> writes:
>
>> Toke Høiland-Jørgensen <toke@redhat.com> writes:
>>
>>> YueHaibing <yuehaibing@huawei.com> writes:
>>>
>>>> Fixes gcc '-Wunused-but-set-variable' warning:
>>>>
>>>> drivers/net/wireless/ath/ath9k/recv.c: In function 'ath_rx_count_airtime':
>>>> drivers/net/wireless/ath/ath9k/recv.c:1010:18: warning:
>>>> variable 'acq' set but not used [-Wunused-but-set-variable]
>>>>
>>>> It's not used after 89cea7493a34 ("ath9k: Switch to mac80211 TXQ scheduling
>>>> and airtime APIs"). Also remove related variables.
>>>
>>> Ah, right, seems I forgot to clean that up. I wonder why I didn't get a
>>> compiler warning for it.
>>
>> I think the warning is not enabled by default and you need to use W=1
>> Makefile variable to enable it.
>
> Hmm, right, thanks! Guess I should get into the habit of compiling with
> warnings enabled before submitting patches :)
But you might get a lot of warnings and it could be difficult to find
new warnings from all the noise. In ath10k I just fixed all W=1 warnings
which I saw with gcc 8.1 and only now I was able to enable W=1 on my
build script.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] ath9k: remove set but not used variable 'acq'
From: Toke Høiland-Jørgensen @ 2019-02-25 11:04 UTC (permalink / raw)
To: Kalle Valo
Cc: YueHaibing, QCA ath9k Development, linux-wireless, netdev,
kernel-janitors
In-Reply-To: <877edom8vl.fsf@kamboji.qca.qualcomm.com>
Kalle Valo <kvalo@codeaurora.org> writes:
> Toke Høiland-Jørgensen <toke@redhat.com> writes:
>
>> Kalle Valo <kvalo@codeaurora.org> writes:
>>
>>> Toke Høiland-Jørgensen <toke@redhat.com> writes:
>>>
>>>> YueHaibing <yuehaibing@huawei.com> writes:
>>>>
>>>>> Fixes gcc '-Wunused-but-set-variable' warning:
>>>>>
>>>>> drivers/net/wireless/ath/ath9k/recv.c: In function 'ath_rx_count_airtime':
>>>>> drivers/net/wireless/ath/ath9k/recv.c:1010:18: warning:
>>>>> variable 'acq' set but not used [-Wunused-but-set-variable]
>>>>>
>>>>> It's not used after 89cea7493a34 ("ath9k: Switch to mac80211 TXQ scheduling
>>>>> and airtime APIs"). Also remove related variables.
>>>>
>>>> Ah, right, seems I forgot to clean that up. I wonder why I didn't get a
>>>> compiler warning for it.
>>>
>>> I think the warning is not enabled by default and you need to use W=1
>>> Makefile variable to enable it.
>>
>> Hmm, right, thanks! Guess I should get into the habit of compiling with
>> warnings enabled before submitting patches :)
>
> But you might get a lot of warnings and it could be difficult to find
> new warnings from all the noise. In ath10k I just fixed all W=1 warnings
> which I saw with gcc 8.1 and only now I was able to enable W=1 on my
> build script.
Ah, that was what that patch series was about :)
Right, well, I guess I'll give it a shot at some point and if it's not
too annoying I'll try to keep an eye on the warnings output...
-Toke
^ permalink raw reply
* [PATCH net-next v2 0/5] net/mlx5e: Make little improvement for mlx5e
From: xiangxia.m.yue @ 2019-02-25 10:40 UTC (permalink / raw)
To: saeedm, gerlitz.or; +Cc: netdev, Tonghao Zhang
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This serial patches are not bugfixes, and just little improvement for mlx5e.
v1->v2
Patch 1: remove the duplicated error messages and stick to extack only
Patch 2: use the 'unknown' instead of empty string
Patch 5: is new patch
Tonghao Zhang (5):
net/mlx5e: Return -EOPNOTSUPP when modify header action zero
net/mlx5e: Make the log friendly when decapsulation offload not
supported
net/mlx5e: Remove 'parse_attr' argument in parse_tc_fdb_actions()
net/mlx5e: Deletes unnecessary setting of esw_attr->parse_attr
net/mlx5e: Return -EOPNOTSUPP when attempting to offload an
unsupported action
.../net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 8 +++++---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 23 +++++++++++++++-------
2 files changed, 21 insertions(+), 10 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH net-next v2 1/5] net/mlx5e: Return -EOPNOTSUPP when modify header action zero
From: xiangxia.m.yue @ 2019-02-25 10:40 UTC (permalink / raw)
To: saeedm, gerlitz.or; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1551091207-10366-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
When max modify header action is zero, we return -EOPNOTSUPP
directly. In this way, we can ignore wrong message info (e.g.
"mlx5: parsed 0 pedit actions, can't do more").
This happens when offloading pedit actions on mlx VFs.
For example:
$ tc filter add dev mlx5_vf parent ffff: protocol ip prio 1 \
flower skip_sw dst_mac 00:10:56:fb:64:e8 \
dst_ip 1.1.1.100 src_ip 1.1.1.200 \
action pedit ex munge eth src set 00:10:56:b4:5d:20
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index b38986e..708f819 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2002,7 +2002,8 @@ static int offload_pedit_fields(struct pedit_headers_action *hdrs,
static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
struct pedit_headers_action *hdrs,
int namespace,
- struct mlx5e_tc_flow_parse_attr *parse_attr)
+ struct mlx5e_tc_flow_parse_attr *parse_attr,
+ struct netlink_ext_ack *extack)
{
int nkeys, action_size, max_actions;
@@ -2015,6 +2016,12 @@ static int alloc_mod_hdr_actions(struct mlx5e_priv *priv,
else /* namespace is MLX5_FLOW_NAMESPACE_KERNEL - NIC offloading */
max_actions = MLX5_CAP_FLOWTABLE_NIC_RX(priv->mdev, max_modify_header_actions);
+ if (!max_actions) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "don't support pedit actions, can't offload");
+ return -EOPNOTSUPP;
+ }
+
/* can get up to crazingly 16 HW actions in 32 bits pedit SW key */
max_actions = min(max_actions, nkeys * 16);
@@ -2072,7 +2079,8 @@ static int alloc_tc_pedit_action(struct mlx5e_priv *priv, int namespace,
u8 cmd;
if (!parse_attr->mod_hdr_actions) {
- err = alloc_mod_hdr_actions(priv, hdrs, namespace, parse_attr);
+ err = alloc_mod_hdr_actions(priv, hdrs,
+ namespace, parse_attr, extack);
if (err)
goto out_err;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next v2 2/5] net/mlx5e: Make the log friendly when decapsulation offload not supported
From: xiangxia.m.yue @ 2019-02-25 10:40 UTC (permalink / raw)
To: saeedm, gerlitz.or; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1551091207-10366-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
If we try to offload decapsulation actions to VFs hw, we get the log [1].
It's not friendly, because the kind of net device is null, and we don't
know what '0' means.
[1] "mlx5_core 0000:05:01.2 vf_0: decapsulation offload is not supported for net device (0)"
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
index bdcc5e7..6cbfbfa 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
@@ -84,7 +84,7 @@ static const char *mlx5e_netdev_kind(struct net_device *dev)
if (dev->rtnl_link_ops)
return dev->rtnl_link_ops->kind;
else
- return "";
+ return "unknown";
}
static int mlx5e_route_lookup_ipv6(struct mlx5e_priv *priv,
@@ -620,8 +620,10 @@ int mlx5e_tc_tun_parse(struct net_device *filter_dev,
headers_c, headers_v);
} else {
netdev_warn(priv->netdev,
- "decapsulation offload is not supported for %s net device (%d)\n",
- mlx5e_netdev_kind(filter_dev), tunnel_type);
+ "decapsulation offload is not supported for %s (kind: \"%s\")\n",
+ netdev_name(filter_dev),
+ mlx5e_netdev_kind(filter_dev));
+
return -EOPNOTSUPP;
}
return err;
--
1.8.3.1
^ 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