* [PATCH net-next 2/2] tcp_cubic: refine Hystart delay threshold
From: Eric Dumazet @ 2014-12-05 0:13 UTC (permalink / raw)
To: David Miller
Cc: netdev, Nandita Dukkipati, Neal Cardwell, Yuchung Cheng,
Sangtae Ha
From: Eric Dumazet <edumazet@google.com>
In commit 2b4636a5f8ca ("tcp_cubic: make the delay threshold of HyStart
less sensitive"), HYSTART_DELAY_MIN was changed to 4 ms.
The remaining problem is that using delay_min + (delay_min/16) as the
threshold is too sensitive.
6.25 % of variation is too small for rtt above 60 ms, which are not
uncommon.
Lets use 12.5 % instead (delay_min + (delay_min/8))
Tested:
80 ms RTT between peers, FQ/pacing packet scheduler on sender.
10 bulk transfers of 10 seconds :
nstat >/dev/null
for i in `seq 1 10`
do
netperf -H remote -- -k THROUGHPUT | grep THROUGHPUT
done
nstat | grep Hystart
With the 6.25 % threshold :
THROUGHPUT=20.66
THROUGHPUT=249.38
THROUGHPUT=254.10
THROUGHPUT=14.94
THROUGHPUT=251.92
THROUGHPUT=237.73
THROUGHPUT=19.18
THROUGHPUT=252.89
THROUGHPUT=21.32
THROUGHPUT=15.58
TcpExtTCPHystartTrainDetect 2 0.0
TcpExtTCPHystartTrainCwnd 4756 0.0
TcpExtTCPHystartDelayDetect 5 0.0
TcpExtTCPHystartDelayCwnd 180 0.0
With the 12.5 % threshold
THROUGHPUT=251.09
THROUGHPUT=247.46
THROUGHPUT=250.92
THROUGHPUT=248.91
THROUGHPUT=250.88
THROUGHPUT=249.84
THROUGHPUT=250.51
THROUGHPUT=254.15
THROUGHPUT=250.62
THROUGHPUT=250.89
TcpExtTCPHystartTrainDetect 1 0.0
TcpExtTCPHystartTrainCwnd 3175 0.0
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/ipv4/tcp_cubic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index c1d07c7ed03d7d37fa28d1509093e686f78134d2..6b6002416a73950d493661ea1459870f49917efc 100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -393,7 +393,7 @@ static void hystart_update(struct sock *sk, u32 delay)
ca->sample_cnt++;
} else {
if (ca->curr_rtt > ca->delay_min +
- HYSTART_DELAY_THRESH(ca->delay_min>>4)) {
+ HYSTART_DELAY_THRESH(ca->delay_min >> 3)) {
ca->found |= HYSTART_DELAY;
NET_INC_STATS_BH(sock_net(sk),
LINUX_MIB_TCPHYSTARTDELAYDETECT);
^ permalink raw reply related
* [PATCH net-next 1/2] tcp_cubic: add SNMP counters to track how effective is Hystart
From: Eric Dumazet @ 2014-12-05 0:13 UTC (permalink / raw)
To: David Miller
Cc: netdev, Nandita Dukkipati, Neal Cardwell, Yuchung Cheng,
Sangtae Ha
From: Eric Dumazet <edumazet@google.com>
When deploying FQ pacing, one thing we noticed is that CUBIC Hystart
triggers too soon.
Having SNMP counters to have an idea of how often the various Hystart
methods trigger is useful prior to any modifications.
This patch adds SNMP counters tracking, how many time "ack train" or
"Delay" based Hystart triggers, and cumulative sum of cwnd at the time
Hystart decided to end SS (Slow Start)
myhost:~# nstat -a | grep Hystart
TcpExtTCPHystartTrainDetect 9 0.0
TcpExtTCPHystartTrainCwnd 20650 0.0
TcpExtTCPHystartDelayDetect 10 0.0
TcpExtTCPHystartDelayCwnd 360 0.0
->
Train detection was triggered 9 times, and average cwnd was
20650/9=2294,
Delay detection was triggered 10 times and average cwnd was 36
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
include/uapi/linux/snmp.h | 4 ++++
net/ipv4/proc.c | 4 ++++
net/ipv4/tcp_cubic.c | 31 ++++++++++++++++++++++---------
3 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
index 30f541b32895..b22224100011 100644
--- a/include/uapi/linux/snmp.h
+++ b/include/uapi/linux/snmp.h
@@ -266,6 +266,10 @@ enum
LINUX_MIB_TCPWANTZEROWINDOWADV, /* TCPWantZeroWindowAdv */
LINUX_MIB_TCPSYNRETRANS, /* TCPSynRetrans */
LINUX_MIB_TCPORIGDATASENT, /* TCPOrigDataSent */
+ LINUX_MIB_TCPHYSTARTTRAINDETECT, /* TCPHystartTrainDetect */
+ LINUX_MIB_TCPHYSTARTTRAINCWND, /* TCPHystartTrainCwnd */
+ LINUX_MIB_TCPHYSTARTDELAYDETECT, /* TCPHystartDelayDetect */
+ LINUX_MIB_TCPHYSTARTDELAYCWND, /* TCPHystartDelayCwnd */
__LINUX_MIB_MAX
};
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index 6513ade8d6dc..8f9cd200ce20 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -288,6 +288,10 @@ static const struct snmp_mib snmp4_net_list[] = {
SNMP_MIB_ITEM("TCPWantZeroWindowAdv", LINUX_MIB_TCPWANTZEROWINDOWADV),
SNMP_MIB_ITEM("TCPSynRetrans", LINUX_MIB_TCPSYNRETRANS),
SNMP_MIB_ITEM("TCPOrigDataSent", LINUX_MIB_TCPORIGDATASENT),
+ SNMP_MIB_ITEM("TCPHystartTrainDetect", LINUX_MIB_TCPHYSTARTTRAINDETECT),
+ SNMP_MIB_ITEM("TCPHystartTrainCwnd", LINUX_MIB_TCPHYSTARTTRAINCWND),
+ SNMP_MIB_ITEM("TCPHystartDelayDetect", LINUX_MIB_TCPHYSTARTDELAYDETECT),
+ SNMP_MIB_ITEM("TCPHystartDelayCwnd", LINUX_MIB_TCPHYSTARTDELAYCWND),
SNMP_MIB_SENTINEL
};
diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index 20de0118c98e..c1d07c7ed03d 100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -363,16 +363,28 @@ static void hystart_update(struct sock *sk, u32 delay)
struct tcp_sock *tp = tcp_sk(sk);
struct bictcp *ca = inet_csk_ca(sk);
- if (!(ca->found & hystart_detect)) {
+ if (ca->found & hystart_detect)
+ return;
+
+ if (hystart_detect & HYSTART_ACK_TRAIN) {
u32 now = bictcp_clock();
/* first detection parameter - ack-train detection */
if ((s32)(now - ca->last_ack) <= hystart_ack_delta) {
ca->last_ack = now;
- if ((s32)(now - ca->round_start) > ca->delay_min >> 4)
+ if ((s32)(now - ca->round_start) > ca->delay_min >> 4) {
ca->found |= HYSTART_ACK_TRAIN;
+ NET_INC_STATS_BH(sock_net(sk),
+ LINUX_MIB_TCPHYSTARTTRAINDETECT);
+ NET_ADD_STATS_BH(sock_net(sk),
+ LINUX_MIB_TCPHYSTARTTRAINCWND,
+ tp->snd_cwnd);
+ tp->snd_ssthresh = tp->snd_cwnd;
+ }
}
+ }
+ if (hystart_detect & HYSTART_DELAY) {
/* obtain the minimum delay of more than sampling packets */
if (ca->sample_cnt < HYSTART_MIN_SAMPLES) {
if (ca->curr_rtt == 0 || ca->curr_rtt > delay)
@@ -381,15 +393,16 @@ static void hystart_update(struct sock *sk, u32 delay)
ca->sample_cnt++;
} else {
if (ca->curr_rtt > ca->delay_min +
- HYSTART_DELAY_THRESH(ca->delay_min>>4))
+ HYSTART_DELAY_THRESH(ca->delay_min>>4)) {
ca->found |= HYSTART_DELAY;
+ NET_INC_STATS_BH(sock_net(sk),
+ LINUX_MIB_TCPHYSTARTDELAYDETECT);
+ NET_ADD_STATS_BH(sock_net(sk),
+ LINUX_MIB_TCPHYSTARTDELAYCWND,
+ tp->snd_cwnd);
+ tp->snd_ssthresh = tp->snd_cwnd;
+ }
}
- /*
- * Either one of two conditions are met,
- * we exit from slow start immediately.
- */
- if (ca->found & hystart_detect)
- tp->snd_ssthresh = tp->snd_cwnd;
}
}
^ permalink raw reply related
* Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
From: Joe Perches @ 2014-12-04 23:35 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alexei Starovoitov, Quentin Lambert, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
x86, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1417734891.22424.2.camel@edumazet-glaptop2.roam.corp.google.com>
On Thu, 2014-12-04 at 15:14 -0800, Eric Dumazet wrote:
> On Thu, 2014-12-04 at 15:00 -0800, Joe Perches wrote:
> > Use the (1 << reg) & mask trick to reduce code size.
[]
> Really, the root cause of this is the 'inline' abuse in non fast paths
> for non trivial functions.
There is no object size change with is_ereg()
defined "static inline" or "static"
Curiously, if you mark it noinline, the size increases.
gcc 4.9.1, x86-64, -O2 no profiling support
$ size arch/x86/net/bpf_jit_comp.o.st*
text data bss dec hex filename
10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.static_inline
11535 4 0 11539 2d13 arch/x86/net/bpf_jit_comp.o.static_noinline
10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.static_without_inline
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
From: Alexei Starovoitov @ 2014-12-04 23:31 UTC (permalink / raw)
To: Eric Dumazet
Cc: Joe Perches, Quentin Lambert, David S. Miller, Alexey Kuznetsov,
James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, x86@kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1417734891.22424.2.camel@edumazet-glaptop2.roam.corp.google.com>
On Thu, Dec 4, 2014 at 3:14 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2014-12-04 at 15:00 -0800, Joe Perches wrote:
>> Use the (1 << reg) & mask trick to reduce code size.
>>
>> x86-64 size difference -O2 without profiling for various
>> gcc versions:
>
>>
>> Signed-off-by: Joe Perches <joe@perches.com>
>> ---
>>
>> compiled, untested by me, but per Alexei Starovoitov this passes
>> the test_bpf suite
>
> Really, the root cause of this is the 'inline' abuse in non fast paths
> for non trivial functions.
well, it is a trivial function even from compiler point of view.
Dropping inline keyword doesn't help. gcc still inlines them.
Changing all 3 functions to _noinline_ doesn't help either.
So I think this patch is actually quite helpful to reduce code size.
^ permalink raw reply
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Jesse Gross @ 2014-12-04 23:23 UTC (permalink / raw)
To: Du Fan
Cc: Pravin Shelar, Thomas Graf, Michael S. Tsirkin, Du, Fan,
Jason Wang, netdev@vger.kernel.org, davem@davemloft.net,
fw@strlen.de, dev@openvswitch.org
In-Reply-To: <548011DE.4000208@gmail.com>
On Wed, Dec 3, 2014 at 11:48 PM, Du Fan <fengyuleidian0615@gmail.com> wrote:
> 于 2014年12月04日 06:51, Jesse Gross 写道:
>>
>> My proposal would be something like this:
>> * For L2, reduce the VM MTU to the lowest common denominator on the
>> segment.
>> * For L3, use path MTU discovery or fragment inner packet (i.e.
>> normal routing behavior).
>> * As a last resort (such as if using an old version of virtio in the
>> guest), fragment the tunnel packet.
>
>
> After some investigation on OpenvSwitch package, it seems before this
> commit: 06021dcb "datapath: compat: Fix compilation 3.11" OpenvSwitch
> package is doing GSO on its own.
>
> rpl_ip_local_out
> -> tnl_skb_gso_segment
> ^^^^^^^^^^^^^^^
> Perform GSO in above function
> -> ip_local_out
> .
> .
> -> ip_finish_output
>
> Which means, when over-MTU-sized skb enter ip_local_out, it's not a gso
> type skb, so the stack perform ip fragmentation, and send them out.
> So, over-MTU-sized skb did travel through stack into outside.
>
> Why not dropping such OpenvSwitch level GSO operation after 3.10?
The change in 3.11 was that the tunnel infrastructure used by OVS was
upstreamed and shared by all implementations. It's not right to
perform GSO in OVS itself as it prevents the logic from being used by
other components. Breaking up the packet in OVS also eliminates some
of the benefits of GSO by shortening the optimized path and prevents
offloading to hardware.
^ permalink raw reply
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Jesse Gross @ 2014-12-04 23:19 UTC (permalink / raw)
To: Thomas Graf
Cc: Michael S. Tsirkin, Du, Fan, Jason Wang, netdev@vger.kernel.org,
davem@davemloft.net, fw@strlen.de, dev@openvswitch.org,
Pravin Shelar
In-Reply-To: <20141204092650.GA13660@casper.infradead.org>
On Thu, Dec 4, 2014 at 1:26 AM, Thomas Graf <tgraf@suug.ch> wrote:
> On 12/03/14 at 05:51pm, Jesse Gross wrote:
>> I think it depends on where you put the PMTU check. If routing is
>> happening in OVS where it is decomposed in several discrete actions
>> like set MAC and decrement TTL then perhaps there is another explicit
>> action to check the MTU. If it is happening in the context of the IP
>> stack, then ICMP generation occurs automatically and if you get that
>> if you write a flow to send a packet there. In each case, it seems
>> like a flow would be steering you by way of an action to do routing so
>> you would have fine grained control. I don't see this as conflicting
>> with L3 ACLs in an L2 context in the same way that you don't have to
>> automatically decrement the TTL.
>
> OK, I was under the impression that you would detect L3 behaviour
> desire in OVS without an explicit action. Hence the worry on wrong
> classification for L3 ACLs.
>
> Whether we mark the packet and defer the MTU check or we check right
> in the action is an implementation detail in the end. I think we
> agree on concept level that we need an API to control behaviour.
Yes, I agree.
^ permalink raw reply
* Re: 3.12.33 - BUG xfrm_selector_match+0x25/0x2f6
From: Julian Anastasov @ 2014-12-04 23:15 UTC (permalink / raw)
To: Steffen Klassert
Cc: Smart Weblications GmbH - Florian Wiessner, netdev, LKML, stable
In-Reply-To: <20141204075627.GE6390@secunet.com>
Hello,
On Thu, 4 Dec 2014, Steffen Klassert wrote:
> > [16623.096721] Call Trace:
> > [16623.096744] <IRQ>
> > [16623.096749] [<ffffffff81547a7c>] ? xfrm_sk_policy_lookup+0x44/0x9b
> > [16623.096802] [<ffffffff81547ef7>] ? xfrm_lookup+0x91/0x446
> > [16623.096832] [<ffffffff81541316>] ? ip_route_me_harder+0x150/0x1b0
> > [16623.096865] [<ffffffffa01b6457>] ? ip_vs_route_me_harder+0x86/0x91 [ip_vs]
> > [16623.096899] [<ffffffffa01b797a>] ? ip_vs_out+0x2d3/0x5bc [ip_vs]
> > [16623.096930] [<ffffffff81501420>] ? ip_rcv_finish+0x2b8/0x2b8
>
> I really wonder why the xfrm_sk_policy_lookup codepath is taken here.
> It looks like this is the processing of an inbound ipv4 packet that
> is going to be rerouted to the output path by ipvs, so this packet
> should not have socket context at all.
In above trace looks like IPVS-NAT is used between
local client and some real server. IPVS handles this skb
at LOCAL_IN and calls ip_vs_route_me_harder(). If we have
skb->sk at LOCAL_IN, my first thought is about early demux.
If I remember correctly, looking at commit f5a41847acc535e2
("ipvs: move ip_route_me_harder for ICMP") that introduced
this rerouting (2.6.37), it was needed because at that time TCP
used rt_src from received skb to select daddr in ip_send_reply().
As packets to server are DNAT-ed and packets to client are
SNAT-ed we used rerouting to fill rt_src with correct IP
after SNAT.
Now when routing cache is removed in 3.6 and
tcp_v4_send_reset() is changed to provide ip_hdr(skb)->saddr
instead of rt_src it should be safe to remove this rerouting,
it is enough that ip_hdr(skb)->saddr was updated on IPVS-SNAT at
LOCAL_IN. In fact, rt_src was removed early in 3.0 with
commit 0a5ebb8000c5362 ("ipv4: Pass explicit daddr arg to
ip_send_reply().").
This is only to explain above stack. Not sure
if problem is related somehow to early demux but such
commits look interesting:
- commit 6b8dbcf2c44fd7a ("bridge: netfilter: orphan skb before invoking
ip netfilter hooks")
Also, it would be good to know which 3.x kernel between
3.13 and 3.17 fixes the problem, it will narrow the search.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
From: Eric Dumazet @ 2014-12-04 23:14 UTC (permalink / raw)
To: Joe Perches
Cc: Alexei Starovoitov, Quentin Lambert, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
x86, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1417734048.2721.22.camel@perches.com>
On Thu, 2014-12-04 at 15:00 -0800, Joe Perches wrote:
> Use the (1 << reg) & mask trick to reduce code size.
>
> x86-64 size difference -O2 without profiling for various
> gcc versions:
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>
> compiled, untested by me, but per Alexei Starovoitov this passes
> the test_bpf suite
Really, the root cause of this is the 'inline' abuse in non fast paths
for non trivial functions.
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
From: Alexei Starovoitov @ 2014-12-04 23:12 UTC (permalink / raw)
To: Joe Perches, Daniel Borkmann
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86@kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1417734048.2721.22.camel@perches.com>
On Thu, Dec 4, 2014 at 3:00 PM, Joe Perches <joe@perches.com> wrote:
> Use the (1 << reg) & mask trick to reduce code size.
>
> x86-64 size difference -O2 without profiling for various
> gcc versions:
>
> $ size arch/x86/net/bpf_jit_comp.o*
> text data bss dec hex filename
> 9266 4 0 9270 2436 arch/x86/net/bpf_jit_comp.o.4.4.new
> 10042 4 0 10046 273e arch/x86/net/bpf_jit_comp.o.4.4.old
> 9109 4 0 9113 2399 arch/x86/net/bpf_jit_comp.o.4.6.new
> 9717 4 0 9721 25f9 arch/x86/net/bpf_jit_comp.o.4.6.old
> 8789 4 0 8793 2259 arch/x86/net/bpf_jit_comp.o.4.7.new
> 10245 4 0 10249 2809 arch/x86/net/bpf_jit_comp.o.4.7.old
> 9671 4 0 9675 25cb arch/x86/net/bpf_jit_comp.o.4.9.new
> 10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.4.9.old
>
> Signed-off-by: Joe Perches <joe@perches.com>
probably it was worth noting in comment that
reg is 4-bit value and AUX_REG==12, so it won't overflow.
Dave, it's for net-next of course.
Suggested-by: Alexei Starovoitov <ast@plumgrid.com>
Tested-by: Alexei Starovoitov <ast@plumgrid.com>
^ permalink raw reply
* [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
From: Joe Perches @ 2014-12-04 23:00 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAADnVQKtYkKtsd9rUFm2yfMoh7F_SRFPVP8hpvSY00KpmB-NNg@mail.gmail.com>
Use the (1 << reg) & mask trick to reduce code size.
x86-64 size difference -O2 without profiling for various
gcc versions:
$ size arch/x86/net/bpf_jit_comp.o*
text data bss dec hex filename
9266 4 0 9270 2436 arch/x86/net/bpf_jit_comp.o.4.4.new
10042 4 0 10046 273e arch/x86/net/bpf_jit_comp.o.4.4.old
9109 4 0 9113 2399 arch/x86/net/bpf_jit_comp.o.4.6.new
9717 4 0 9721 25f9 arch/x86/net/bpf_jit_comp.o.4.6.old
8789 4 0 8793 2259 arch/x86/net/bpf_jit_comp.o.4.7.new
10245 4 0 10249 2809 arch/x86/net/bpf_jit_comp.o.4.7.old
9671 4 0 9675 25cb arch/x86/net/bpf_jit_comp.o.4.9.new
10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.4.9.old
Signed-off-by: Joe Perches <joe@perches.com>
---
compiled, untested by me, but per Alexei Starovoitov this passes
the test_bpf suite
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 3f62734..09e2cea 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -135,11 +135,11 @@ static const int reg2hex[] = {
*/
static inline bool is_ereg(u32 reg)
{
- if (reg == BPF_REG_5 || reg == AUX_REG ||
- (reg >= BPF_REG_7 && reg <= BPF_REG_9))
- return true;
- else
- return false;
+ return (1 << reg) & (BIT(BPF_REG_5) |
+ BIT(AUX_REG) |
+ BIT(BPF_REG_7) |
+ BIT(BPF_REG_8) |
+ BIT(BPF_REG_9));
}
/* add modifiers if 'reg' maps to x64 registers r8..r15 */
^ permalink raw reply related
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: Joe Perches @ 2014-12-04 22:45 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet,
LKML, kernel-janitors, Julia Lawall
In-Reply-To: <5480DFB5.4090708@users.sourceforge.net>
On Thu, 2014-12-04 at 23:27 +0100, SF Markus Elfring wrote:
> >> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> > []
> >> @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
> >> setup_sg(sg_in, state->sha1_digest, state->keylen);
> >> setup_sg(sg_out, state->session_key, state->keylen);
> >> if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
> >> - state->keylen) != 0) {
> >> - printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
> >> - }
> >> + state->keylen) != 0)
> >> + pr_warn("mppe_rekey: cipher_encrypt failed\n");
> >
> > It's generally nicer to replace embedded function names
> > with "%s: ", __func__
> >
> > pr_warn("%s: cipher_encrypt failed\n", __func__);
>
> Do you want that I send a third patch series for the fine-tuning of these parameters?
If you want.
I just wanted you to be aware of it for future patches.
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Alexei Starovoitov @ 2014-12-04 22:44 UTC (permalink / raw)
To: Joe Perches
Cc: David Laight, Quentin Lambert, David S. Miller, Alexey Kuznetsov,
James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, x86@kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
On Thu, Dec 4, 2014 at 10:05 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2014-12-04 at 07:56 -0800, Alexei Starovoitov wrote:
>> On Thu, Dec 4, 2014 at 1:26 AM, Joe Perches <joe@perches.com> wrote:
>> > On Thu, 2014-11-27 at 10:49 -0800, Joe Perches wrote:
>> >> On Thu, 2014-11-27 at 12:25 +0000, David Laight wrote:
>> >> > Why the change in data?
>> >>
>> >> btw: without gcov and using -O2
>> >>
>> >> $ size arch/x86/net/bpf_jit_comp.o*
>> >> text data bss dec hex filename
>> >> 9671 4 0 9675 25cb arch/x86/net/bpf_jit_comp.o.new
>> >> 10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.old
>> >
>> > Alexei?
>> >
>> > Is this 10% reduction in size a good reason to change the code?
>>
>> yes.
>> I believe you're seeing it with gcc 4.9. I wanted to double
>> check what 4.6 and 4.7 are doing. If they're not suddenly
>> increase code size then resubmit it for inclusion please.
>
> I get these sizes for these compilers
> (x86-64, -O2, without profiling)
>
> $ size arch/x86/net/bpf_jit_comp.o*
> text data bss dec hex filename
> 9266 4 0 9270 2436 arch/x86/net/bpf_jit_comp.o.4.4.new
> 10042 4 0 10046 273e arch/x86/net/bpf_jit_comp.o.4.4.old
> 9109 4 0 9113 2399 arch/x86/net/bpf_jit_comp.o.4.6.new
> 9717 4 0 9721 25f9 arch/x86/net/bpf_jit_comp.o.4.6.old
> 8789 4 0 8793 2259 arch/x86/net/bpf_jit_comp.o.4.7.new
> 10245 4 0 10249 2809 arch/x86/net/bpf_jit_comp.o.4.7.old
> 9671 4 0 9675 25cb arch/x86/net/bpf_jit_comp.o.4.9.new
> 10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.4.9.old
>
> I am a bit surprised by the size variations
yeah. the difference is surprising.
Just tried with 4.7 and my regular config and I see the same difference.
Looks like gcc wasn't able to fold conditions into cmov
and used a bunch of cmp/jmp
Since is_ereg() was inlined ~70 times on its own and as
part of other functions, the difference of 3-4 instructions
may a large difference in total size.
test_bpf also passes, so please resubmit properly.
^ permalink raw reply
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: SF Markus Elfring @ 2014-12-04 22:27 UTC (permalink / raw)
To: Joe Perches
Cc: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet,
LKML, kernel-janitors, Julia Lawall
In-Reply-To: <1417731809.2721.17.camel@perches.com>
>> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> []
>> @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
>> setup_sg(sg_in, state->sha1_digest, state->keylen);
>> setup_sg(sg_out, state->session_key, state->keylen);
>> if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
>> - state->keylen) != 0) {
>> - printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
>> - }
>> + state->keylen) != 0)
>> + pr_warn("mppe_rekey: cipher_encrypt failed\n");
>
> It's generally nicer to replace embedded function names
> with "%s: ", __func__
>
> pr_warn("%s: cipher_encrypt failed\n", __func__);
Do you want that I send a third patch series for the fine-tuning of these parameters?
Regards,
Martkus
^ permalink raw reply
* Re: [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: Joe Perches @ 2014-12-04 22:23 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet,
LKML, kernel-janitors, Julia Lawall
In-Reply-To: <5480DBDE.7040604@users.sourceforge.net>
On Thu, 2014-12-04 at 23:10 +0100, SF Markus Elfring wrote:
> The mppe_rekey() function contained a few update candidates.
> * Curly brackets were still used around a single function call "printk".
> * Unwanted space characters
>
> Let us improve these implementation details according to the current Linux
> coding style convention.
trivia:
> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
[]
> @@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
> setup_sg(sg_in, state->sha1_digest, state->keylen);
> setup_sg(sg_out, state->session_key, state->keylen);
> if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
> - state->keylen) != 0) {
> - printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
> - }
> + state->keylen) != 0)
> + pr_warn("mppe_rekey: cipher_encrypt failed\n");
It's generally nicer to replace embedded function names
with "%s: ", __func__
pr_warn("%s: cipher_encrypt failed\n", __func__);
^ permalink raw reply
* [PATCH v2 6/6] net-PPP: Delete another unnecessary assignment in mppe_alloc()
From: SF Markus Elfring @ 2014-12-04 22:20 UTC (permalink / raw)
To: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <5480DA32.8000201@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:42:30 +0100
The data structure element "sha1" was assigned a null pointer by the
mppe_alloc() after a function call "crypto_alloc_hash" failed.
It was determined that this element was not accessed by the implementation
of the crypto_free_blkcipher() function.
Let us delete it from the affected implementation because the element "sha1"
will not be accessible outside the function after the detected
allocation failure.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/ppp/ppp_mppe.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index b7db4b1..32cb054 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -208,10 +208,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
goto out_free;
state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
- if (IS_ERR(state->sha1)) {
- state->sha1 = NULL;
+ if (IS_ERR(state->sha1))
goto out_free_blkcipher;
- }
digestsize = crypto_hash_digestsize(state->sha1);
if (digestsize < MPPE_MAX_KEY_LEN)
--
2.1.3
^ permalink raw reply related
* [PATCH v2 5/6] net-PPP: Delete an unnecessary assignment in mppe_alloc()
From: SF Markus Elfring @ 2014-12-04 22:18 UTC (permalink / raw)
To: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <5480DA32.8000201@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:33:34 +0100
The data structure element "arc4" was assigned a null pointer by the
mppe_alloc() function if a previous function call "crypto_alloc_blkcipher"
failed. This assignment became unnecessary with previous source
code adjustments.
Let us delete it from the affected implementation because the element "arc4"
will not be accessible outside the function after the detected
allocation failure.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/ppp/ppp_mppe.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index c82198f..b7db4b1 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -204,10 +204,8 @@ static void *mppe_alloc(unsigned char *options, int optlen)
state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
- if (IS_ERR(state->arc4)) {
- state->arc4 = NULL;
+ if (IS_ERR(state->arc4))
goto out_free;
- }
state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(state->sha1)) {
--
2.1.3
^ permalink raw reply related
* [PATCH v2 4/6] net-PPP: Less function calls in mppe_alloc() after error detection
From: SF Markus Elfring @ 2014-12-04 22:16 UTC (permalink / raw)
To: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <5480DA32.8000201@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:30:20 +0100
The functions crypto_free_blkcipher((), crypto_free_hash() and kfree() could be
called in some cases by the mppe_alloc() function during error handling even
if the passed data structure element contained still a null pointer.
This implementation detail could be improved by adjustments for jump labels.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/ppp/ppp_mppe.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 94ff216..c82198f 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -196,11 +196,11 @@ static void *mppe_alloc(unsigned char *options, int optlen)
if (optlen != CILEN_MPPE + sizeof(state->master_key) ||
options[0] != CI_MPPE || options[1] != CILEN_MPPE)
- goto out;
+ return NULL;
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (state == NULL)
- goto out;
+ return NULL;
state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
@@ -212,16 +212,16 @@ static void *mppe_alloc(unsigned char *options, int optlen)
state->sha1 = crypto_alloc_hash("sha1", 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(state->sha1)) {
state->sha1 = NULL;
- goto out_free;
+ goto out_free_blkcipher;
}
digestsize = crypto_hash_digestsize(state->sha1);
if (digestsize < MPPE_MAX_KEY_LEN)
- goto out_free;
+ goto out_free_hash;
state->sha1_digest = kmalloc(digestsize, GFP_KERNEL);
if (!state->sha1_digest)
- goto out_free;
+ goto out_free_hash;
/* Save keys. */
memcpy(state->master_key, &options[CILEN_MPPE],
@@ -236,14 +236,12 @@ static void *mppe_alloc(unsigned char *options, int optlen)
return (void *)state;
+out_free_hash:
+ crypto_free_hash(state->sha1);
+out_free_blkcipher:
+ crypto_free_blkcipher(state->arc4);
out_free:
- kfree(state->sha1_digest);
- if (state->sha1)
- crypto_free_hash(state->sha1);
- if (state->arc4)
- crypto_free_blkcipher(state->arc4);
kfree(state);
-out:
return NULL;
}
--
2.1.3
^ permalink raw reply related
* [PATCH v2 3/6] net-PPP: Deletion of unnecessary checks before the function call "kfree"
From: SF Markus Elfring @ 2014-12-04 22:15 UTC (permalink / raw)
To: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <5480DA32.8000201@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:22:23 +0100
The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/ppp/ppp_mppe.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index b80af29..94ff216 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -237,8 +237,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
return (void *)state;
out_free:
- if (state->sha1_digest)
- kfree(state->sha1_digest);
+ kfree(state->sha1_digest);
if (state->sha1)
crypto_free_hash(state->sha1);
if (state->arc4)
@@ -255,8 +254,7 @@ static void mppe_free(void *arg)
{
struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
if (state) {
- if (state->sha1_digest)
- kfree(state->sha1_digest);
+ kfree(state->sha1_digest);
if (state->sha1)
crypto_free_hash(state->sha1);
if (state->arc4)
--
2.1.3
^ permalink raw reply related
* [PATCH v2 2/6] net-PPP: Fix indentation
From: SF Markus Elfring @ 2014-12-04 22:13 UTC (permalink / raw)
To: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <5480DA32.8000201@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:15:20 +0100
The implementations of the functions "mppe_alloc" and "mppe_free" contained
unwanted space characters.
Let us improve the indentation according to the Linux coding style convention.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/ppp/ppp_mppe.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 84b7bce..b80af29 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -236,15 +236,15 @@ static void *mppe_alloc(unsigned char *options, int optlen)
return (void *)state;
- out_free:
- if (state->sha1_digest)
+out_free:
+ if (state->sha1_digest)
kfree(state->sha1_digest);
- if (state->sha1)
+ if (state->sha1)
crypto_free_hash(state->sha1);
- if (state->arc4)
+ if (state->arc4)
crypto_free_blkcipher(state->arc4);
- kfree(state);
- out:
+ kfree(state);
+out:
return NULL;
}
@@ -255,13 +255,13 @@ static void mppe_free(void *arg)
{
struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
if (state) {
- if (state->sha1_digest)
- kfree(state->sha1_digest);
- if (state->sha1)
- crypto_free_hash(state->sha1);
- if (state->arc4)
- crypto_free_blkcipher(state->arc4);
- kfree(state);
+ if (state->sha1_digest)
+ kfree(state->sha1_digest);
+ if (state->sha1)
+ crypto_free_hash(state->sha1);
+ if (state->arc4)
+ crypto_free_blkcipher(state->arc4);
+ kfree(state);
}
}
--
2.1.3
^ permalink raw reply related
* [PATCH v2 1/6] net-PPP: Replacement of a printk() call by pr_warn() in mppe_rekey()
From: SF Markus Elfring @ 2014-12-04 22:10 UTC (permalink / raw)
To: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <5480DA32.8000201@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 18:28:52 +0100
The mppe_rekey() function contained a few update candidates.
* Curly brackets were still used around a single function call "printk".
* Unwanted space characters
Let us improve these implementation details according to the current Linux
coding style convention.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/ppp/ppp_mppe.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 911b216..84b7bce 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -172,9 +172,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
setup_sg(sg_in, state->sha1_digest, state->keylen);
setup_sg(sg_out, state->session_key, state->keylen);
if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
- state->keylen) != 0) {
- printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
- }
+ state->keylen) != 0)
+ pr_warn("mppe_rekey: cipher_encrypt failed\n");
} else {
memcpy(state->session_key, state->sha1_digest, state->keylen);
}
--
2.1.3
^ permalink raw reply related
* Re: [patch iproute2 1/6] iproute2: ipa: show switch id
From: Thomas Graf @ 2014-12-04 22:07 UTC (permalink / raw)
To: Jiri Pirko
Cc: Eric W. Biederman, netdev, davem, nhorman, andy, dborkman,
ogerlitz, jesse, pshelar, azhou, ben, stephen, jeffrey.t.kirsher,
vyasevic, xiyou.wangcong, john.r.fastabend, edumazet, jhs,
sfeldma, f.fainelli, roopa, linville, jasowang, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, gospo, b
In-Reply-To: <20141204211041.GN1861@nanopsycho.orion>
On 12/04/14 at 10:10pm, Jiri Pirko wrote:
> Thu, Dec 04, 2014 at 09:55:07PM CET, ebiederm@xmission.com wrote:
> >My intuition says we want something like ifindex, but I am not at all
> >certain how switch id is planned to be used. Given that it is single
> >box I don't expect you are sending it out over the wire.
>
> No, it is not to be send out.
We discussed this before and stated we would fix this as soon as it's
merged. Let's just define a simple and quick switch id prefix generator
ala ifindex which serves as a unique prefix to all switch ids.
I have time early next week if Jiri doesn't.
^ permalink raw reply
* [PATCH v2 0/6] net-PPP: Deletion of a few unnecessary checks
From: SF Markus Elfring @ 2014-12-04 22:03 UTC (permalink / raw)
To: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev, Eric Dumazet
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <547CA157.1080401@cogentembedded.com>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 4 Dec 2014 22:50:28 +0100
Further update suggestions were taken into account before and after a patch
was applied from static source code analysis.
Markus Elfring (6):
Replacement of a printk() call by pr_warn() in mppe_rekey()
Fix indentation
Deletion of unnecessary checks before the function call "kfree"
Less function calls in mppe_alloc() after error detection
Delete an unnecessary assignment in mppe_alloc()
Delete another unnecessary assignment in mppe_alloc()
drivers/net/ppp/ppp_mppe.c | 49 +++++++++++++++++++---------------------------
1 file changed, 20 insertions(+), 29 deletions(-)
--
2.1.3
^ permalink raw reply
* Re: [PATCH iproute2] ip monitor: Fixed printing timestamp few times
From: Vadim Kochan @ 2014-12-04 21:43 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev@vger.kernel.org
In-Reply-To: <20141203174153.GA12034@angus-think.wlc.globallogic.com>
Hi Stephen,
Seems that problem is that by default ipmonitor subscribes to the all
RTNL groups except RTMGRP_TC:
groups = ~RTMGRP_TC;
And I assume that other netlink messages can also cause such
Timestamping w/o event message ...
So I think there should be another way than ~RTMGR_TC, and this big IF
in accept_msg can be removed.
Regards,
Vadim
^ permalink raw reply
* Re: [patch iproute2 1/6] iproute2: ipa: show switch id
From: Eric W. Biederman @ 2014-12-04 21:24 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, jhs, sfeldma,
f.fainelli, roopa, linville, jasowang, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck, john.ronciak,
mleitner, shrijeet, gospo, bcrl, hemal
In-Reply-To: <20141204211041.GN1861@nanopsycho.orion>
Jiri Pirko <jiri@resnulli.us> writes:
>>No. phys_port_id is not broken in the same way, and phys_port_id does
>>not have the same set of properties.
>>
>>phys_port_id's in practice all have an IEEE prefix that identifies the
>>manufacturer and a manufacture assigned serial number. Aka a mac
>>address or a EUID-64. What the mlx4 ethernet driver is doing retunring
>>a 64bit EUID-64 I don't know. If there are problems in the worst
>>case issues with phys_port_id are fixable by simple driver tweaks,
>>because fundamentally we are working with globally uniuqe identifiers.
>>Well globally unique baring manufacturing bugs in eeproms.
>
> Well the fact that phys_post_id's are now implemented mostly by putting
> mac into it does not mean that other drivers cannot do it differently.
> So once again, phys_port_id and phys_switch_id are the same in this
> matter.
No exclusively implemented that way.
And yes other drivers can implement bugs, that doesn't mean those bugs
will be toleraged and won't break userspace.
>>I agree with you that the switch id concept can be saved. But I think
>>we should fix switch id before we export it to userspace so we don't
>>have to break userspace later.
>>
>>My intuition says we want something like ifindex, but I am not at all
>>certain how switch id is planned to be used. Given that it is single
>>box I don't expect you are sending it out over the wire.
>
> No, it is not to be send out.
>
>>
>>*shrug*
>>
>>Why does switch id need to be persistent? Why can't switch id be
>>property like ifindex?
>
> Well I can imagine that multiple ports of the same switch chip could be
> passed through to the virtual machines (similar to SR-IOV pf/vf).
In that case you do need something that is globally unique. Because
if I migrate your virtual machine onto a different physical machine
seeing the same switch id and I might make the mistaken assumption
that I am remain on the same switch if the ids are not global.
>>What are the actual requirements.
>
>
> They are actually very similar to phys_port_id. Therefore I made that
> the same.
Then please for rocket use a non-buggy implementation with a globally
unique id.
Given your descriptions of the requirements I can't see how any other
implementation isn't buggy.
Eric
^ permalink raw reply
* Re: [patch] ipvs: uninitialized data with IP_VS_IPV6
From: Julian Anastasov @ 2014-12-04 21:19 UTC (permalink / raw)
To: Dan Carpenter
Cc: Wensong Zhang, Simon Horman, Pablo Neira Ayuso, Patrick McHardy,
Jozsef Kadlecsik, David S. Miller, netdev, lvs-devel,
netfilter-devel, coreteam, kernel-janitors
In-Reply-To: <20141203101213.GC29583@mwanda>
Hello,
On Wed, 3 Dec 2014, Dan Carpenter wrote:
> The app_tcp_pkt_out() function expects "*diff" to be set and ends up
> using uninitialized data if CONFIG_IP_VS_IPV6 is turned on.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This bug is very old.
I guess ip_vs_ftp_in() needs the same fix?
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
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