* Re: [PATCH net-next 1/5] gianfar: Cleanup device refs in gfar_private
From: Claudiu Manoil @ 2013-02-12 17:14 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: netdev, David S. Miller
In-Reply-To: <511A57AA.8070004@windriver.com>
On 2/12/2013 4:54 PM, Paul Gortmaker wrote:
> On 13-02-12 07:47 AM, Claudiu Manoil wrote:
>> * remove unused device_node pointer
>> * remove duplicate SET_NETDEV_DEV()
>> * use device pointer (dev) to simplify the code and to
>> avoid double indirections (esp. on the "fast path")
>
> Ideally, when you find yourself making a list within the longlog,
> that is a hint that you might want to start making it into
> multiple commits, for ease of review. Granted #1 and #2 are
> trivial, but #3 probably could be a separate commit. Did you
> see any change in the object size or the disassembly when
> making change #3?
>
> P.
I didn't inspect the assembly code yet, but this should definitely
generate better, faster code. I don't see why it wouldn't...
Thanks,
Claudiu
^ permalink raw reply
* RE: [net-next 04/10] igb: Fix for sparse warning in igb_get_i2c_client
From: Wyborny, Carolyn @ 2013-02-12 17:12 UTC (permalink / raw)
To: Ben Hutchings, Kirsher, Jeffrey T
Cc: davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com,
sassmann@redhat.com
In-Reply-To: <1360602355.2701.5.camel@bwh-desktop.uk.solarflarecom.com>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Ben Hutchings
> Sent: Monday, February 11, 2013 9:06 AM
> To: Kirsher, Jeffrey T; Wyborny, Carolyn
> Cc: davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com
> Subject: Re: [net-next 04/10] igb: Fix for sparse warning in igb_get_i2c_client
>
> On Fri, 2013-02-08 at 02:39 -0800, Jeff Kirsher wrote:
> > From: Carolyn Wyborny <carolyn.wyborny@intel.com>
> >
> > This patch changes definition of i2c_client in igb_get_i2c_client to
> > static to prevent sparse warning.
>
> So, in order to fix the warning:
>
> drivers/net/ethernet/intel/igb/igb_main.c:7611:19: warning: symbol
> 'igb_get_i2c_client' was not declared. Should it be static?
[..]
And this was not the warning I was trying to fix with the static change. The sparse warning was "cast to
restricted __le64." Not sure why you are seeing that particular warning, but I will look for it in my next patch on this function.
Thanks,
Carolyn
Carolyn Wyborny
Linux Development
Networking Division
Intel Corporation
^ permalink raw reply
* Re: [PATCH] checksum: remove duplicated line of code in csum_fold
From: David Miller @ 2013-02-12 17:12 UTC (permalink / raw)
To: pablo; +Cc: netdev, arnd
In-Reply-To: <1360689003-21794-1-git-send-email-pablo@netfilter.org>
From: pablo@netfilter.org
Date: Tue, 12 Feb 2013 18:10:03 +0100
> From: Pablo Neira Ayuso <pablo@netfilter.org>
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
I am very sure that those two lines are there intentionally.
The first addition one can overflow, therefore we need to
fold again (which cannot overflow).
^ permalink raw reply
* [PATCH] checksum: remove duplicated line of code in csum_fold
From: pablo @ 2013-02-12 17:10 UTC (permalink / raw)
To: netdev; +Cc: davem, Arnd Bergmann
From: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/asm-generic/checksum.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/asm-generic/checksum.h b/include/asm-generic/checksum.h
index c084767..f68b2ea 100644
--- a/include/asm-generic/checksum.h
+++ b/include/asm-generic/checksum.h
@@ -51,7 +51,6 @@ static inline __sum16 csum_fold(__wsum csum)
{
u32 sum = (__force u32)csum;
sum = (sum & 0xffff) + (sum >> 16);
- sum = (sum & 0xffff) + (sum >> 16);
return (__force __sum16)~sum;
}
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH net-next 3/5] gianfar: GRO_DROP is unlikely
From: Claudiu Manoil @ 2013-02-12 17:05 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: netdev, David S. Miller
In-Reply-To: <20130212163056.GA1000@windriver.com>
On 2/12/2013 6:30 PM, Paul Gortmaker wrote:
> [[PATCH net-next 3/5] gianfar: GRO_DROP is unlikely] On 12/02/2013 (Tue 14:47) Claudiu Manoil wrote:
>
>> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
>> ---
>> drivers/net/ethernet/freescale/gianfar.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
>> index 096fb5f..5622134 100644
>> --- a/drivers/net/ethernet/freescale/gianfar.c
>> +++ b/drivers/net/ethernet/freescale/gianfar.c
>> @@ -2745,7 +2745,7 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
>> /* Send the packet up the stack */
>> ret = napi_gro_receive(napi, skb);
>>
>> - if (GRO_DROP == ret)
>> + if (unlikely(GRO_DROP == ret))
>> priv->extra_stats.kernel_dropped++;
>>
>> return 0;
>
> I wondered about this, specifically if it was a moot point, when the
> actual unlikely was deployed right at the end of the fcn. It turns out
> that it does make a difference, since gfar_process_frame gets inlined,
> and so the increment code gets moved out of line (I have marked the if
> statment with * and the increment code within "-----"):
>
> ------------------------- as is currently ------------------
> 4d14: 80 61 00 18 lwz r3,24(r1)
> 4d18: 7f c4 f3 78 mr r4,r30
> 4d1c: 48 00 00 01 bl 4d1c <gfar_clean_rx_ring+0x10c>
> * 4d20: 2f 83 00 04 cmpwi cr7,r3,4
> 4d24: 40 9e 00 1c bne- cr7,4d40 <gfar_clean_rx_ring+0x130>
> ----------------------------
> 4d28: 81 3c 01 f8 lwz r9,504(r28)
> 4d2c: 81 5c 01 fc lwz r10,508(r28)
> 4d30: 31 4a 00 01 addic r10,r10,1
> 4d34: 7d 29 01 94 addze r9,r9
> 4d38: 91 3c 01 f8 stw r9,504(r28)
> 4d3c: 91 5c 01 fc stw r10,508(r28)
> ----------------------------
> 4d40: a0 1f 00 24 lhz r0,36(r31)
> 4d44: 81 3f 00 00 lwz r9,0(r31)
> 4d48: 7f a4 eb 78 mr r4,r29
> 4d4c: 7f e3 fb 78 mr r3,r31
>
>
> -------------------------- unlikely ------------------------
> 4d14: 80 61 00 18 lwz r3,24(r1)
> 4d18: 7f c4 f3 78 mr r4,r30
> 4d1c: 48 00 00 01 bl 4d1c <gfar_clean_rx_ring+0x10c>
> * 4d20: 2f 83 00 04 cmpwi cr7,r3,4
> 4d24: 41 9e 03 94 beq- cr7,50b8 <gfar_clean_rx_ring+0x4a8>
> 4d28: a0 1f 00 24 lhz r0,36(r31)
> 4d2c: 81 3f 00 00 lwz r9,0(r31)
> 4d30: 7f a4 eb 78 mr r4,r29
> 4d34: 7f e3 fb 78 mr r3,r31
> [...]
> 50b8: 81 3c 01 f8 lwz r9,504(r28)
> 50bc: 81 5c 01 fc lwz r10,508(r28)
> 50c0: 31 4a 00 01 addic r10,r10,1
> 50c4: 7d 29 01 94 addze r9,r9
> 50c8: 91 3c 01 f8 stw r9,504(r28)
> 50cc: 91 5c 01 fc stw r10,508(r28)
> 50d0: 4b ff fc 58 b 4d28 <gfar_clean_rx_ring+0x118>
>
> So, the increment does actually get moved ~1k away. Maybe you can
> incorporate the above information in your long log, so the next guy
> doesn't wonder about the same question I did.
>
> Also, I noticed that gfar_process_frame() can be void instead of int.
> It never returns anything but zero, and the return code is ignored at
> the single call site. Maybe you can add a patch to your series for that
> as well?
>
> Paul.
>
> .
Thanks for the notice.
The slightest code changes to gfar_process_frame() are reflected
to the driver's performance (i.e. throughput). So this is a very
"performance sensitive" function.
I'll see what happens if changed to return void.
Claudiu
^ permalink raw reply
* Re: [PATCH v2] net: fec_mpc52xx: Read MAC address from device-tree
From: David Miller @ 2013-02-12 17:05 UTC (permalink / raw)
To: R65777; +Cc: sr, netdev, linuxppc-dev, agust
In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D065A0271@039-SN2MPN1-023.039d.mgd.msft.net>
From: Bhushan Bharat-R65777 <R65777@freescale.com>
Date: Tue, 12 Feb 2013 10:56:05 +0000
> Why we read from MAC registers if Linux should not rely on bootloader?
Because it used to and if you just remove that code then you break
existing setups, and I explicitly told him to code things this way.
^ permalink raw reply
* Re: [PATCH net-next 2/5] gianfar: Cleanup and optimize struct gfar_private
From: Paul Gortmaker @ 2013-02-12 16:49 UTC (permalink / raw)
To: Claudiu Manoil; +Cc: netdev, David S. Miller
In-Reply-To: <511A66A3.7040704@freescale.com>
On 13-02-12 10:58 AM, Claudiu Manoil wrote:
> On 2/12/2013 5:11 PM, Paul Gortmaker wrote:
>> On 13-02-12 07:47 AM, Claudiu Manoil wrote:
>>> * group run-time critical fields within the 1st cacheline
>>> followed by the tx|rx_queue reference arrays and the interrupt
>>> group instances (gfargrp) (all cacheline aligned)
>>> * change 'padding' from unsigned short to u16
>>> * clear 20+ byte memory hole
>>
>> Per prev. mail, it gets harder to see which change is where,
>> when they are all lumped together like this. For example, it
>> wasn't obvious to me where the 20 byte hole was. Also, it
>> doesn't look like you changed the padding, but rather instead
>> totally re-purposed it, leaving no alignment padding after the
>> uchar bitfields (where it was originally).
>>
>> P.
>
> The 20 byte hole was here:
>
> struct gfar_private {
> unsigned int num_tx_queues;
> unsigned int num_rx_queues;
> unsigned int num_grps;
> unsigned int mode;
> unsigned int total_tx_ring_size;
> unsigned int total_rx_ring_size;
> struct device_node * node;
> struct net_device * ndev;
> /* --- cacheline 1 boundary (32 bytes) --- */
> struct platform_device * ofdev;
> enum gfar_errata errata;
>
> /* XXX 24 bytes hole, try to pack */
OK, so this is good information, that wasn't in your commit log.
(Is it 20 or 24 though? I'm guessing 24, and original commit
log was wrong in saying 20.)
Something like:
"The default gcc layout of gfar_private leaves an implicit 24
byte hole (bytes XYZ --> ABC in the 1st cache line) after the
errata enum...."
>
> /* --- cacheline 2 boundary (64 bytes) --- */
> struct gfar_priv_grp gfargrp[2];
>
>
> At the end of the patch series the first cacheline is without holes.
> Please note that the re-grouping of members and their order is most
> important. For instance why keep in the first cacheline something
The importance of ordering and alignment was never in question. What
was missing, was conveying to the reviewer exactly how you'd made it
better, without forcing them to manually deconstruct gfar_priv in the
before and after cases, esp. when you'd already done that analysis.
> as unimportant as total_rx_ring_size? Members like rx_buffer_size,
> or padding, or even errata, are critical however for the fast path.
> Rx processing (gfar_poll + clean_rx_ring) is the bottleneck here,
> keeping the CPU to 100%. So the main goal is to optimize this path,
> including memory access/cache optimizations. For instance, better
> results were obtained by inverting rx|tx_queue[] with gfargrp[], originally:
> /* --- cacheline 1 boundary (32 bytes) --- */
> struct gfar_priv_tx_q * tx_queue[8]; /* 32 32 */
> /* --- cacheline 2 boundary (64 bytes) --- */
> struct gfar_priv_rx_q * rx_queue[8]; /* 64 32 */
> /* --- cacheline 3 boundary (96 bytes) --- */
> struct gfar_priv_grp gfargrp[2]; /* 96 192 */
>
> The uchar bitfields are unimportant here (used at "init time"), and
> they take 4 bytes including padding anyway. So whether it's uchar or
> uint, it's the same, maybe better left uchar to discourage the abuse
> of these bitfields. (A 2-3byte hole here doesn't change anything
> to the whole structure size, which is padded to be at least a 32B
> multiple.)
This here too, i.e. why the padding after the bitfields is unimportant
and hence removed should also be in the long log.
Thanks,
Paul.
--
>
> Thanks,
> Claudiu
>
>
^ permalink raw reply
* Re: [PATCH net-next 3/5] gianfar: GRO_DROP is unlikely
From: Eric Dumazet @ 2013-02-12 16:47 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: Claudiu Manoil, netdev, David S. Miller
In-Reply-To: <20130212163056.GA1000@windriver.com>
On Tue, 2013-02-12 at 11:30 -0500, Paul Gortmaker wrote:
> [...]
> 50b8: 81 3c 01 f8 lwz r9,504(r28)
> 50bc: 81 5c 01 fc lwz r10,508(r28)
> 50c0: 31 4a 00 01 addic r10,r10,1
> 50c4: 7d 29 01 94 addze r9,r9
> 50c8: 91 3c 01 f8 stw r9,504(r28)
> 50cc: 91 5c 01 fc stw r10,508(r28)
> 50d0: 4b ff fc 58 b 4d28 <gfar_clean_rx_ring+0x118>
What I can see here is the counter is 64bit and arch is 32bit,
and no sync is used.
So ethtool -S has races.
^ permalink raw reply
* Re: [patch net-next v5 11/11] act_police: remove <=mtu check for gso skbs
From: Eric Dumazet @ 2013-02-12 16:40 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <1360663929-1023-12-git-send-email-jiri@resnulli.us>
On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
> This check made bigger packets incorrectly dropped. Remove this
> limitation for gso skbs.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> net/sched/act_police.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index 823463a..2dba297 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -325,7 +325,7 @@ static int tcf_act_police(struct sk_buff *skb, const struct tc_action *a,
> return police->tcf_action;
> }
>
> - if (qdisc_pkt_len(skb) <= police->tcfp_mtu) {
> + if (qdisc_pkt_len(skb) <= police->tcfp_mtu || skb_is_gso(skb)) {
> if (!police->rate_present) {
> spin_unlock(&police->tcf_lock);
> return police->tcfp_result;
> @@ -336,7 +336,7 @@ static int tcf_act_police(struct sk_buff *skb, const struct tc_action *a,
> police->tcfp_burst);
> if (police->peak_present) {
> ptoks = toks + police->tcfp_ptoks;
> - if (ptoks > police->tcfp_mtu_ptoks)
> + if (ptoks > police->tcfp_mtu_ptoks && !skb_is_gso(skb))
> ptoks = police->tcfp_mtu_ptoks;
> ptoks -= (s64) psched_l2t_ns(&police->peak,
> qdisc_pkt_len(skb));
Same remark here : This chunks looks wrong to me.
^ permalink raw reply
* Re: [patch net-next v5 10/11] tbf: take into account gso skbs
From: Eric Dumazet @ 2013-02-12 16:39 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <1360663929-1023-11-git-send-email-jiri@resnulli.us>
On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
> Ignore max_size check for gso skbs. This check made bigger packets
> incorrectly dropped. Remove this limitation for gso skbs.
>
> Also for peaks, ignore mtu for gso skbs.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> net/sched/sch_tbf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
> index c8388f3..8973e93 100644
> --- a/net/sched/sch_tbf.c
> +++ b/net/sched/sch_tbf.c
> @@ -121,7 +121,7 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
> struct tbf_sched_data *q = qdisc_priv(sch);
> int ret;
>
> - if (qdisc_pkt_len(skb) > q->max_size)
> + if (qdisc_pkt_len(skb) > q->max_size && !skb_is_gso(skb))
> return qdisc_reshape_fail(skb, sch);
>
> ret = qdisc_enqueue(skb, q->qdisc);
> @@ -165,7 +165,7 @@ static struct sk_buff *tbf_dequeue(struct Qdisc *sch)
>
> if (q->peak_present) {
> ptoks = toks + q->ptokens;
> - if (ptoks > q->mtu)
> + if (ptoks > q->mtu && !skb_is_gso(skb))
> ptoks = q->mtu;
> ptoks -= (s64) psched_l2t_ns(&q->peak, len);
> }
I guess this part is wrong.
If we dont cap ptoks to q->mtu we allow bigger bursts.
Ideally we could re-segment the skb if psched_l2t_ns(&q->peak, len) is
bigger than q->mtu
^ permalink raw reply
* Re: [patch net-next v5 08/11] act_police: move struct tcf_police to act_police.c
From: Eric Dumazet @ 2013-02-12 16:34 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <1360663929-1023-9-git-send-email-jiri@resnulli.us>
On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
> It's not used anywhere else, so move it.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [patch net-next v5 07/11] tbf: improved accuracy at high rates
From: Eric Dumazet @ 2013-02-12 16:34 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <1360663929-1023-8-git-send-email-jiri@resnulli.us>
On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
> Current TBF uses rate table computed by the "tc" userspace program,
> which has the following issue:
>
> The rate table has 256 entries to map packet lengths to
> token (time units). With TSO sized packets, the 256 entry granularity
> leads to loss/gain of rate, making the token bucket inaccurate.
>
> Thus, instead of relying on rate table, this patch explicitly computes
> the time and accounts for packet transmission times with nanosecond
> granularity.
>
> This is a followup to 56b765b79e9a78dc7d3f8850ba5e5567205a3ecd
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [patch net-next v5 06/11] sch_api: introduce qdisc_watchdog_schedule_ns()
From: Eric Dumazet @ 2013-02-12 16:32 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <1360663929-1023-7-git-send-email-jiri@resnulli.us>
On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
> tbf will need to schedule watchdog in ns. No need to convert it twice.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> include/net/pkt_sched.h | 10 ++++++++--
> net/sched/sch_api.c | 6 +++---
> 2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
> index 66f5ac3..388bf8b 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -65,8 +65,14 @@ struct qdisc_watchdog {
> };
>
> extern void qdisc_watchdog_init(struct qdisc_watchdog *wd, struct Qdisc *qdisc);
> -extern void qdisc_watchdog_schedule(struct qdisc_watchdog *wd,
> - psched_time_t expires);
> +extern void qdisc_watchdog_schedule_ns(struct qdisc_watchdog *wd, u64 expires);
> +
> +static inline void qdisc_watchdog_schedule(struct qdisc_watchdog *wd,
> + psched_time_t expires)
> +{
> + qdisc_watchdog_schedule_ns(wd, PSCHED_TICKS2NS(expires));
> +}
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* Re: [PATCH net-next 3/5] gianfar: GRO_DROP is unlikely
From: Paul Gortmaker @ 2013-02-12 16:30 UTC (permalink / raw)
To: Claudiu Manoil; +Cc: netdev, David S. Miller
In-Reply-To: <1360673237-349-3-git-send-email-claudiu.manoil@freescale.com>
[[PATCH net-next 3/5] gianfar: GRO_DROP is unlikely] On 12/02/2013 (Tue 14:47) Claudiu Manoil wrote:
> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
> ---
> drivers/net/ethernet/freescale/gianfar.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index 096fb5f..5622134 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -2745,7 +2745,7 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
> /* Send the packet up the stack */
> ret = napi_gro_receive(napi, skb);
>
> - if (GRO_DROP == ret)
> + if (unlikely(GRO_DROP == ret))
> priv->extra_stats.kernel_dropped++;
>
> return 0;
I wondered about this, specifically if it was a moot point, when the
actual unlikely was deployed right at the end of the fcn. It turns out
that it does make a difference, since gfar_process_frame gets inlined,
and so the increment code gets moved out of line (I have marked the if
statment with * and the increment code within "-----"):
------------------------- as is currently ------------------
4d14: 80 61 00 18 lwz r3,24(r1)
4d18: 7f c4 f3 78 mr r4,r30
4d1c: 48 00 00 01 bl 4d1c <gfar_clean_rx_ring+0x10c>
* 4d20: 2f 83 00 04 cmpwi cr7,r3,4
4d24: 40 9e 00 1c bne- cr7,4d40 <gfar_clean_rx_ring+0x130>
----------------------------
4d28: 81 3c 01 f8 lwz r9,504(r28)
4d2c: 81 5c 01 fc lwz r10,508(r28)
4d30: 31 4a 00 01 addic r10,r10,1
4d34: 7d 29 01 94 addze r9,r9
4d38: 91 3c 01 f8 stw r9,504(r28)
4d3c: 91 5c 01 fc stw r10,508(r28)
----------------------------
4d40: a0 1f 00 24 lhz r0,36(r31)
4d44: 81 3f 00 00 lwz r9,0(r31)
4d48: 7f a4 eb 78 mr r4,r29
4d4c: 7f e3 fb 78 mr r3,r31
-------------------------- unlikely ------------------------
4d14: 80 61 00 18 lwz r3,24(r1)
4d18: 7f c4 f3 78 mr r4,r30
4d1c: 48 00 00 01 bl 4d1c <gfar_clean_rx_ring+0x10c>
* 4d20: 2f 83 00 04 cmpwi cr7,r3,4
4d24: 41 9e 03 94 beq- cr7,50b8 <gfar_clean_rx_ring+0x4a8>
4d28: a0 1f 00 24 lhz r0,36(r31)
4d2c: 81 3f 00 00 lwz r9,0(r31)
4d30: 7f a4 eb 78 mr r4,r29
4d34: 7f e3 fb 78 mr r3,r31
[...]
50b8: 81 3c 01 f8 lwz r9,504(r28)
50bc: 81 5c 01 fc lwz r10,508(r28)
50c0: 31 4a 00 01 addic r10,r10,1
50c4: 7d 29 01 94 addze r9,r9
50c8: 91 3c 01 f8 stw r9,504(r28)
50cc: 91 5c 01 fc stw r10,508(r28)
50d0: 4b ff fc 58 b 4d28 <gfar_clean_rx_ring+0x118>
So, the increment does actually get moved ~1k away. Maybe you can
incorporate the above information in your long log, so the next guy
doesn't wonder about the same question I did.
Also, I noticed that gfar_process_frame() can be void instead of int.
It never returns anything but zero, and the return code is ignored at
the single call site. Maybe you can add a patch to your series for that
as well?
Paul.
^ permalink raw reply
* Re: [PATCH] net: fix infinite loop in __skb_recv_datagram()
From: Pavel Emelyanov @ 2013-02-12 16:18 UTC (permalink / raw)
To: Eric Dumazet, David Miller; +Cc: Tommi Rantala, netdev, Dave Jones
In-Reply-To: <1360685813.13993.12.camel@edumazet-glaptop>
On 02/12/2013 08:16 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Tommi was fuzzing with trinity and reported the following problem :
>
> commit 3f518bf745 (datagram: Add offset argument to __skb_recv_datagram)
> missed that a raw socket receive queue can contain skbs with no payload.
>
> We can loop in __skb_recv_datagram() with MSG_PEEK mode, because
> wait_for_packet() is not prepared to skip these skbs.
>
> [ 83.541011] INFO: rcu_sched detected stalls on CPUs/tasks: {}
> (detected by 0, t=26002 jiffies, g=27673, c=27672, q=75)
> [ 83.541011] INFO: Stall ended before state dump start
> [ 108.067010] BUG: soft lockup - CPU#0 stuck for 22s! [trinity-child31:2847]
> ...
> [ 108.067010] Call Trace:
> [ 108.067010] [<ffffffff818cc103>] __skb_recv_datagram+0x1a3/0x3b0
> [ 108.067010] [<ffffffff818cc33d>] skb_recv_datagram+0x2d/0x30
> [ 108.067010] [<ffffffff819ed43d>] rawv6_recvmsg+0xad/0x240
> [ 108.067010] [<ffffffff818c4b04>] sock_common_recvmsg+0x34/0x50
> [ 108.067010] [<ffffffff818bc8ec>] sock_recvmsg+0xbc/0xf0
> [ 108.067010] [<ffffffff818bf31e>] sys_recvfrom+0xde/0x150
> [ 108.067010] [<ffffffff81ca4329>] system_call_fastpath+0x16/0x1b
>
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> Tested-by: Tommi Rantala <tt.rantala@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Pavel Emelyanov <xemul@parallels.com>
Acked-by: Pavel Emelyanov <xemul@parallels.com>
Thanks!
> ---
> net/core/datagram.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index 0337e2b..368f9c3 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -187,7 +187,7 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
> skb_queue_walk(queue, skb) {
> *peeked = skb->peeked;
> if (flags & MSG_PEEK) {
> - if (*off >= skb->len) {
> + if (*off >= skb->len && skb->len) {
> *off -= skb->len;
> continue;
> }
>
>
> .
>
^ permalink raw reply
* [PATCH] net: fix infinite loop in __skb_recv_datagram()
From: Eric Dumazet @ 2013-02-12 16:16 UTC (permalink / raw)
To: Tommi Rantala, David Miller; +Cc: netdev, Dave Jones, Pavel Emelyanov
In-Reply-To: <CA+ydwtoBttwiVz+oy0hnetUTQMi7XHyS9bfGzjuP1L41srBk=g@mail.gmail.com>
From: Eric Dumazet <edumazet@google.com>
Tommi was fuzzing with trinity and reported the following problem :
commit 3f518bf745 (datagram: Add offset argument to __skb_recv_datagram)
missed that a raw socket receive queue can contain skbs with no payload.
We can loop in __skb_recv_datagram() with MSG_PEEK mode, because
wait_for_packet() is not prepared to skip these skbs.
[ 83.541011] INFO: rcu_sched detected stalls on CPUs/tasks: {}
(detected by 0, t=26002 jiffies, g=27673, c=27672, q=75)
[ 83.541011] INFO: Stall ended before state dump start
[ 108.067010] BUG: soft lockup - CPU#0 stuck for 22s! [trinity-child31:2847]
...
[ 108.067010] Call Trace:
[ 108.067010] [<ffffffff818cc103>] __skb_recv_datagram+0x1a3/0x3b0
[ 108.067010] [<ffffffff818cc33d>] skb_recv_datagram+0x2d/0x30
[ 108.067010] [<ffffffff819ed43d>] rawv6_recvmsg+0xad/0x240
[ 108.067010] [<ffffffff818c4b04>] sock_common_recvmsg+0x34/0x50
[ 108.067010] [<ffffffff818bc8ec>] sock_recvmsg+0xbc/0xf0
[ 108.067010] [<ffffffff818bf31e>] sys_recvfrom+0xde/0x150
[ 108.067010] [<ffffffff81ca4329>] system_call_fastpath+0x16/0x1b
Reported-by: Tommi Rantala <tt.rantala@gmail.com>
Tested-by: Tommi Rantala <tt.rantala@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
---
net/core/datagram.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/datagram.c b/net/core/datagram.c
index 0337e2b..368f9c3 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -187,7 +187,7 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
skb_queue_walk(queue, skb) {
*peeked = skb->peeked;
if (flags & MSG_PEEK) {
- if (*off >= skb->len) {
+ if (*off >= skb->len && skb->len) {
*off -= skb->len;
continue;
}
^ permalink raw reply related
* pull request: wireless 2013-02-12
From: John W. Linville @ 2013-02-12 16:03 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 6762 bytes --]
Dave,
Here is another handful of late-breaking fixes intended for the 3.8
stream... Hopefully the will still make it! :-)
There are three mac80211 fixes pulled from Johannes:
"Here are three fixes still for the 3.8 stream, the fix from Cong Ding
for the bad sizeof (Stephen Hemminger had pointed it out before but I'd
promptly forgotten), a mac80211 managed-mode channel context usage fix
where a downgrade would never stop until reaching non-HT and a bug in
the channel determination that could cause invalid channels like HT40+
on channel 11 to be used."
Also included is a mwl8k fix that avoids an oops when using mwl8k
devices that only support the 5 GHz band.
Please let me know if there are problems!
John
---
The following changes since commit 547b4e718115eea74087e28d7fa70aec619200db:
bridge: set priority of STP packets (2013-02-11 14:16:52 -0500)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless.git for-davem
for you to fetch changes up to 318d86dbe55cbc63a61a83b9ff6cdbc044905f5e:
Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless into for-davem (2013-02-12 10:41:46 -0500)
----------------------------------------------------------------
Cong Ding (1):
mac80211: fix error in sizeof() usage
Johannes Berg (2):
mac80211: fix managed mode channel context use
mac80211: fix channel selection bug
John W. Linville (2):
Merge branch 'for-john' of git://git.kernel.org/.../jberg/mac80211
Merge branch 'master' of git://git.kernel.org/.../linville/wireless into for-davem
Jonas Gorski (1):
mwl8k: fix band for supported channels
drivers/net/wireless/mwl8k.c | 36 ++++++++++++++++++------------------
net/mac80211/cfg.c | 3 ++-
net/mac80211/mlme.c | 11 +++++++----
3 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index 83564d3..a00a03e 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -318,20 +318,20 @@ struct mwl8k_sta {
#define MWL8K_STA(_sta) ((struct mwl8k_sta *)&((_sta)->drv_priv))
static const struct ieee80211_channel mwl8k_channels_24[] = {
- { .center_freq = 2412, .hw_value = 1, },
- { .center_freq = 2417, .hw_value = 2, },
- { .center_freq = 2422, .hw_value = 3, },
- { .center_freq = 2427, .hw_value = 4, },
- { .center_freq = 2432, .hw_value = 5, },
- { .center_freq = 2437, .hw_value = 6, },
- { .center_freq = 2442, .hw_value = 7, },
- { .center_freq = 2447, .hw_value = 8, },
- { .center_freq = 2452, .hw_value = 9, },
- { .center_freq = 2457, .hw_value = 10, },
- { .center_freq = 2462, .hw_value = 11, },
- { .center_freq = 2467, .hw_value = 12, },
- { .center_freq = 2472, .hw_value = 13, },
- { .center_freq = 2484, .hw_value = 14, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2412, .hw_value = 1, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2417, .hw_value = 2, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2422, .hw_value = 3, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2427, .hw_value = 4, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2432, .hw_value = 5, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2437, .hw_value = 6, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2442, .hw_value = 7, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2447, .hw_value = 8, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2452, .hw_value = 9, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2457, .hw_value = 10, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2462, .hw_value = 11, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2467, .hw_value = 12, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2472, .hw_value = 13, },
+ { .band = IEEE80211_BAND_2GHZ, .center_freq = 2484, .hw_value = 14, },
};
static const struct ieee80211_rate mwl8k_rates_24[] = {
@@ -352,10 +352,10 @@ static const struct ieee80211_rate mwl8k_rates_24[] = {
};
static const struct ieee80211_channel mwl8k_channels_50[] = {
- { .center_freq = 5180, .hw_value = 36, },
- { .center_freq = 5200, .hw_value = 40, },
- { .center_freq = 5220, .hw_value = 44, },
- { .center_freq = 5240, .hw_value = 48, },
+ { .band = IEEE80211_BAND_5GHZ, .center_freq = 5180, .hw_value = 36, },
+ { .band = IEEE80211_BAND_5GHZ, .center_freq = 5200, .hw_value = 40, },
+ { .band = IEEE80211_BAND_5GHZ, .center_freq = 5220, .hw_value = 44, },
+ { .band = IEEE80211_BAND_5GHZ, .center_freq = 5240, .hw_value = 48, },
};
static const struct ieee80211_rate mwl8k_rates_50[] = {
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 516fbc9..0479c64 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2004,7 +2004,8 @@ static int ieee80211_set_mcast_rate(struct wiphy *wiphy, struct net_device *dev,
{
struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
- memcpy(sdata->vif.bss_conf.mcast_rate, rate, sizeof(rate));
+ memcpy(sdata->vif.bss_conf.mcast_rate, rate,
+ sizeof(int) * IEEE80211_NUM_BANDS);
return 0;
}
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index a355292..5107248 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -3400,6 +3400,7 @@ ieee80211_determine_chantype(struct ieee80211_sub_if_data *sdata,
ret = 0;
+out:
while (!cfg80211_chandef_usable(sdata->local->hw.wiphy, chandef,
IEEE80211_CHAN_DISABLED)) {
if (WARN_ON(chandef->width == NL80211_CHAN_WIDTH_20_NOHT)) {
@@ -3408,14 +3409,13 @@ ieee80211_determine_chantype(struct ieee80211_sub_if_data *sdata,
goto out;
}
- ret = chandef_downgrade(chandef);
+ ret |= chandef_downgrade(chandef);
}
if (chandef->width != vht_chandef.width)
sdata_info(sdata,
- "local regulatory prevented using AP HT/VHT configuration, downgraded\n");
+ "capabilities/regulatory prevented using AP HT/VHT configuration, downgraded\n");
-out:
WARN_ON_ONCE(!cfg80211_chandef_valid(chandef));
return ret;
}
@@ -3529,8 +3529,11 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
*/
ret = ieee80211_vif_use_channel(sdata, &chandef,
IEEE80211_CHANCTX_SHARED);
- while (ret && chandef.width != NL80211_CHAN_WIDTH_20_NOHT)
+ while (ret && chandef.width != NL80211_CHAN_WIDTH_20_NOHT) {
ifmgd->flags |= chandef_downgrade(&chandef);
+ ret = ieee80211_vif_use_channel(sdata, &chandef,
+ IEEE80211_CHANCTX_SHARED);
+ }
return ret;
}
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Paul E. McKenney @ 2013-02-12 16:15 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Srivatsa S. Bhat, tglx, peterz, tj, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <20130210195417.GK2666@linux.vnet.ibm.com>
On Sun, Feb 10, 2013 at 11:54:17AM -0800, Paul E. McKenney wrote:
> On Sun, Feb 10, 2013 at 07:06:07PM +0100, Oleg Nesterov wrote:
> > On 02/08, Paul E. McKenney wrote:
>
> [ . . . ]
>
> > > > +static inline void sync_reader(struct percpu_rwlock *pcpu_rwlock,
> > > > + unsigned int cpu)
> > > > +{
> > > > + smp_rmb(); /* Paired with smp_[w]mb() in percpu_read_[un]lock() */
> > >
> > > As I understand it, the purpose of this memory barrier is to ensure
> > > that the stores in drop_writer_signal() happen before the reads from
> > > ->reader_refcnt in reader_uses_percpu_refcnt(), thus preventing the
> > > race between a new reader attempting to use the fastpath and this writer
> > > acquiring the lock. Unless I am confused, this must be smp_mb() rather
> > > than smp_rmb().
> >
> > And note that before sync_reader() we call announce_writer_active() which
> > already adds mb() before sync_all_readers/sync_reader, so this rmb() looks
> > unneeded.
> >
> > But, at the same time, could you confirm that we do not need another mb()
> > after sync_all_readers() in percpu_write_lock() ? I mean, without mb(),
> > can't this reader_uses_percpu_refcnt() LOAD leak into the critical section
> > protected by ->global_rwlock? Then this LOAD can be re-ordered with other
> > memory operations done by the writer.
>
> As soon as I get the rest of the way through Thomas's patchset. ;-)
There is a memory barrier associated with write_lock(), but it is
only required to keep the critical section inside the lock -- and is
permitted to allow stuff outside of the lock to be reordered into the
critical section. So I believe we do indeed need an smp_mb() between
sync_all_readers() and write_lock() in percpu_write_lock().
Good eyes, Oleg!
Thanx, Paul
^ permalink raw reply
* Re: [PATCH net-next 2/5] gianfar: Cleanup and optimize struct gfar_private
From: Claudiu Manoil @ 2013-02-12 15:58 UTC (permalink / raw)
To: Paul Gortmaker; +Cc: netdev, David S. Miller
In-Reply-To: <511A5BA3.4020306@windriver.com>
On 2/12/2013 5:11 PM, Paul Gortmaker wrote:
> On 13-02-12 07:47 AM, Claudiu Manoil wrote:
>> * group run-time critical fields within the 1st cacheline
>> followed by the tx|rx_queue reference arrays and the interrupt
>> group instances (gfargrp) (all cacheline aligned)
>> * change 'padding' from unsigned short to u16
>> * clear 20+ byte memory hole
>
> Per prev. mail, it gets harder to see which change is where,
> when they are all lumped together like this. For example, it
> wasn't obvious to me where the 20 byte hole was. Also, it
> doesn't look like you changed the padding, but rather instead
> totally re-purposed it, leaving no alignment padding after the
> uchar bitfields (where it was originally).
>
> P.
The 20 byte hole was here:
struct gfar_private {
unsigned int num_tx_queues;
unsigned int num_rx_queues;
unsigned int num_grps;
unsigned int mode;
unsigned int total_tx_ring_size;
unsigned int total_rx_ring_size;
struct device_node * node;
struct net_device * ndev;
/* --- cacheline 1 boundary (32 bytes) --- */
struct platform_device * ofdev;
enum gfar_errata errata;
/* XXX 24 bytes hole, try to pack */
/* --- cacheline 2 boundary (64 bytes) --- */
struct gfar_priv_grp gfargrp[2];
At the end of the patch series the first cacheline is without holes.
Please note that the re-grouping of members and their order is most
important. For instance why keep in the first cacheline something
as unimportant as total_rx_ring_size? Members like rx_buffer_size,
or padding, or even errata, are critical however for the fast path.
Rx processing (gfar_poll + clean_rx_ring) is the bottleneck here,
keeping the CPU to 100%. So the main goal is to optimize this path,
including memory access/cache optimizations. For instance, better
results were obtained by inverting rx|tx_queue[] with gfargrp[], originally:
/* --- cacheline 1 boundary (32 bytes) --- */
struct gfar_priv_tx_q * tx_queue[8]; /* 32 32 */
/* --- cacheline 2 boundary (64 bytes) --- */
struct gfar_priv_rx_q * rx_queue[8]; /* 64 32 */
/* --- cacheline 3 boundary (96 bytes) --- */
struct gfar_priv_grp gfargrp[2]; /* 96 192 */
The uchar bitfields are unimportant here (used at "init time"), and
they take 4 bytes including padding anyway. So whether it's uchar or
uint, it's the same, maybe better left uchar to discourage the abuse
of these bitfields. (A 2-3byte hole here doesn't change anything
to the whole structure size, which is padded to be at least a 32B
multiple.)
Thanks,
Claudiu
^ permalink raw reply
* Re: [PATCH net-next v2] net: sctp: remove unused multiple cookie keys
From: Vlad Yasevich @ 2013-02-12 15:43 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, linux-sctp, netdev, Neil Horman, Vlad Yasevich
In-Reply-To: <1360682133-17658-1-git-send-email-dborkman@redhat.com>
On 02/12/2013 10:15 AM, Daniel Borkmann wrote:
> Vlad says: The whole multiple cookie keys code is completely unused
> and has been all this time. Noone uses anything other then the
> secret_key[0] since there is no changeover support anywhere.
>
> Thus, for now clean up its left-over fragments.
>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: Vlad Yasevich <vyasevic@redhat.com>
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
Thanks
-vlad
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> include/net/sctp/constants.h | 2 +-
> include/net/sctp/structs.h | 5 +----
> net/sctp/endpointola.c | 9 ++-------
> net/sctp/sm_make_chunk.c | 31 +++++++------------------------
> 4 files changed, 11 insertions(+), 36 deletions(-)
>
> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
> index c29707d..a7dd5c5 100644
> --- a/include/net/sctp/constants.h
> +++ b/include/net/sctp/constants.h
> @@ -303,7 +303,7 @@ enum { SCTP_MAX_GABS = 16 };
> * to which we will raise the P-MTU.
> */
> #define SCTP_DEFAULT_MINSEGMENT 512 /* MTU size ... if no mtu disc */
> -#define SCTP_HOW_MANY_SECRETS 2 /* How many secrets I keep */
> +
> #define SCTP_SECRET_SIZE 32 /* Number of octets in a 256 bits. */
>
> #define SCTP_SIGNATURE_SIZE 20 /* size of a SLA-1 signature */
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index fdeb85a..0e0f9d2 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -1236,10 +1236,7 @@ struct sctp_endpoint {
> * Discussion in [RFC1750] can be helpful in
> * selection of the key.
> */
> - __u8 secret_key[SCTP_HOW_MANY_SECRETS][SCTP_SECRET_SIZE];
> - int current_key;
> - int last_key;
> - int key_changed_at;
> + __u8 secret_key[SCTP_SECRET_SIZE];
>
> /* digest: This is a digest of the sctp cookie. This field is
> * only used on the receive path when we try to validate
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index 1a9c5fb..73aad3d 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -151,9 +151,7 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
> ep->rcvbuf_policy = net->sctp.rcvbuf_policy;
>
> /* Initialize the secret key used with cookie. */
> - get_random_bytes(&ep->secret_key[0], SCTP_SECRET_SIZE);
> - ep->last_key = ep->current_key = 0;
> - ep->key_changed_at = jiffies;
> + get_random_bytes(ep->secret_key, sizeof(ep->secret_key));
>
> /* SCTP-AUTH extensions*/
> INIT_LIST_HEAD(&ep->endpoint_shared_keys);
> @@ -249,8 +247,6 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
> /* Final destructor for endpoint. */
> static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
> {
> - int i;
> -
> SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>
> /* Free up the HMAC transform. */
> @@ -273,8 +269,7 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
> sctp_inq_free(&ep->base.inqueue);
> sctp_bind_addr_free(&ep->base.bind_addr);
>
> - for (i = 0; i < SCTP_HOW_MANY_SECRETS; ++i)
> - memset(&ep->secret_key[i], 0, SCTP_SECRET_SIZE);
> + memset(ep->secret_key, 0, sizeof(ep->secret_key));
>
> /* Remove and free the port */
> if (sctp_sk(ep->base.sk)->bind_hash)
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index e1c5fc2..a193f3b 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1589,8 +1589,6 @@ static sctp_cookie_param_t *sctp_pack_cookie(const struct sctp_endpoint *ep,
> struct sctp_signed_cookie *cookie;
> struct scatterlist sg;
> int headersize, bodysize;
> - unsigned int keylen;
> - char *key;
>
> /* Header size is static data prior to the actual cookie, including
> * any padding.
> @@ -1650,12 +1648,11 @@ static sctp_cookie_param_t *sctp_pack_cookie(const struct sctp_endpoint *ep,
>
> /* Sign the message. */
> sg_init_one(&sg, &cookie->c, bodysize);
> - keylen = SCTP_SECRET_SIZE;
> - key = (char *)ep->secret_key[ep->current_key];
> desc.tfm = sctp_sk(ep->base.sk)->hmac;
> desc.flags = 0;
>
> - if (crypto_hash_setkey(desc.tfm, key, keylen) ||
> + if (crypto_hash_setkey(desc.tfm, ep->secret_key,
> + sizeof(ep->secret_key)) ||
> crypto_hash_digest(&desc, &sg, bodysize, cookie->signature))
> goto free_cookie;
> }
> @@ -1682,8 +1679,7 @@ struct sctp_association *sctp_unpack_cookie(
> int headersize, bodysize, fixed_size;
> __u8 *digest = ep->digest;
> struct scatterlist sg;
> - unsigned int keylen, len;
> - char *key;
> + unsigned int len;
> sctp_scope_t scope;
> struct sk_buff *skb = chunk->skb;
> struct timeval tv;
> @@ -1718,34 +1714,21 @@ struct sctp_association *sctp_unpack_cookie(
> goto no_hmac;
>
> /* Check the signature. */
> - keylen = SCTP_SECRET_SIZE;
> sg_init_one(&sg, bear_cookie, bodysize);
> - key = (char *)ep->secret_key[ep->current_key];
> desc.tfm = sctp_sk(ep->base.sk)->hmac;
> desc.flags = 0;
>
> memset(digest, 0x00, SCTP_SIGNATURE_SIZE);
> - if (crypto_hash_setkey(desc.tfm, key, keylen) ||
> + if (crypto_hash_setkey(desc.tfm, ep->secret_key,
> + sizeof(ep->secret_key)) ||
> crypto_hash_digest(&desc, &sg, bodysize, digest)) {
> *error = -SCTP_IERROR_NOMEM;
> goto fail;
> }
>
> if (memcmp(digest, cookie->signature, SCTP_SIGNATURE_SIZE)) {
> - /* Try the previous key. */
> - key = (char *)ep->secret_key[ep->last_key];
> - memset(digest, 0x00, SCTP_SIGNATURE_SIZE);
> - if (crypto_hash_setkey(desc.tfm, key, keylen) ||
> - crypto_hash_digest(&desc, &sg, bodysize, digest)) {
> - *error = -SCTP_IERROR_NOMEM;
> - goto fail;
> - }
> -
> - if (memcmp(digest, cookie->signature, SCTP_SIGNATURE_SIZE)) {
> - /* Yikes! Still bad signature! */
> - *error = -SCTP_IERROR_BAD_SIG;
> - goto fail;
> - }
> + *error = -SCTP_IERROR_BAD_SIG;
> + goto fail;
> }
>
> no_hmac:
>
^ permalink raw reply
* Re: [PATCH 1/1] VSOCK: Introduce VM Sockets
From: Andy King @ 2013-02-12 15:21 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: pv-drivers, netdev, linux-kernel, virtualization, gregkh, davem
In-Reply-To: <5118FEA9.2080201@redhat.com>
Hi Gerd,
> > +struct vsock_transport {
...
> Whoa. This has grown *alot*. Care to explain this please? Patch
> creating a Documentation/virtual/vsock.txt would be cool.
Yes, it grew because of the notification stuff, which I'd forgotten
about until I removed the vmci header from the core code. You are
free to use empty functions for these if they don't make any sense
for virtio. The alternative is for us to move the entire body of
vsock_stream_recv/send into the transport, where we can hide the
notification stuff, but it seems like folks will just end up
duplicating a lot of code then.
> stream_has_data + stream_has_space + stream_rcvhiwat look like they
> are needed for buffer management. Details please (especially for
> stream_rcvhiwat).
stream_has_data: Returns amount of data available (in bytes) in the
socket's receive buffer, or -1 if empty.
stream_has_space: Returns amount of space available (in bytes) in the
socket's send buffer, or -1 if full.
stream_rcvhiwat: The upper bound on the socket's receive buffer.
Which technically is the same as the value returned by
get_buffer_size(), so perhaps we could substitute that here and
drop this one.
> What is stream_is_active?
For the VMCI transport, it indicates if the underlying queuepair is
still around (i.e., make sure we haven't torn it down while sleeping
in a blocking send or receive). Perhaps it's not the best name?
> What is *_allow?
It's very basic filtering. We have specific addresses that we don't
allow, and we look for them in the allow() functions. You can just
return true if you like.
> What are all those notify_* calls?
They're to do with signaling and flow-control. Again, they might
not make any sense at all, but it's hard to know without having
written another transport :)
> Why do you need vsock_transport_{send,recv}_notify_data structs?
> Can't this live in vsock_sock->trans?
Those have to be setup on a per-call basis (per-thread), so it's
just easier to have them on the stack of the send/recv calls. If
you think there's a better name, or a better way to allocate them,
I'm all ears.
Thanks!
- Andy
^ permalink raw reply
* Re: [PATCH net-next v2] net: sctp: remove unused multiple cookie keys
From: Neil Horman @ 2013-02-12 15:20 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, linux-sctp, netdev, Vlad Yasevich
In-Reply-To: <1360682133-17658-1-git-send-email-dborkman@redhat.com>
On Tue, Feb 12, 2013 at 04:15:33PM +0100, Daniel Borkmann wrote:
> Vlad says: The whole multiple cookie keys code is completely unused
> and has been all this time. Noone uses anything other then the
> secret_key[0] since there is no changeover support anywhere.
>
> Thus, for now clean up its left-over fragments.
>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: Vlad Yasevich <vyasevic@redhat.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* [PATCH net-next v2] net: sctp: remove unused multiple cookie keys
From: Daniel Borkmann @ 2013-02-12 15:15 UTC (permalink / raw)
To: davem; +Cc: linux-sctp, netdev, Neil Horman, Vlad Yasevich
Vlad says: The whole multiple cookie keys code is completely unused
and has been all this time. Noone uses anything other then the
secret_key[0] since there is no changeover support anywhere.
Thus, for now clean up its left-over fragments.
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Vlad Yasevich <vyasevic@redhat.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
include/net/sctp/constants.h | 2 +-
include/net/sctp/structs.h | 5 +----
net/sctp/endpointola.c | 9 ++-------
net/sctp/sm_make_chunk.c | 31 +++++++------------------------
4 files changed, 11 insertions(+), 36 deletions(-)
diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
index c29707d..a7dd5c5 100644
--- a/include/net/sctp/constants.h
+++ b/include/net/sctp/constants.h
@@ -303,7 +303,7 @@ enum { SCTP_MAX_GABS = 16 };
* to which we will raise the P-MTU.
*/
#define SCTP_DEFAULT_MINSEGMENT 512 /* MTU size ... if no mtu disc */
-#define SCTP_HOW_MANY_SECRETS 2 /* How many secrets I keep */
+
#define SCTP_SECRET_SIZE 32 /* Number of octets in a 256 bits. */
#define SCTP_SIGNATURE_SIZE 20 /* size of a SLA-1 signature */
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index fdeb85a..0e0f9d2 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1236,10 +1236,7 @@ struct sctp_endpoint {
* Discussion in [RFC1750] can be helpful in
* selection of the key.
*/
- __u8 secret_key[SCTP_HOW_MANY_SECRETS][SCTP_SECRET_SIZE];
- int current_key;
- int last_key;
- int key_changed_at;
+ __u8 secret_key[SCTP_SECRET_SIZE];
/* digest: This is a digest of the sctp cookie. This field is
* only used on the receive path when we try to validate
diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index 1a9c5fb..73aad3d 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -151,9 +151,7 @@ static struct sctp_endpoint *sctp_endpoint_init(struct sctp_endpoint *ep,
ep->rcvbuf_policy = net->sctp.rcvbuf_policy;
/* Initialize the secret key used with cookie. */
- get_random_bytes(&ep->secret_key[0], SCTP_SECRET_SIZE);
- ep->last_key = ep->current_key = 0;
- ep->key_changed_at = jiffies;
+ get_random_bytes(ep->secret_key, sizeof(ep->secret_key));
/* SCTP-AUTH extensions*/
INIT_LIST_HEAD(&ep->endpoint_shared_keys);
@@ -249,8 +247,6 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
/* Final destructor for endpoint. */
static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
{
- int i;
-
SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
/* Free up the HMAC transform. */
@@ -273,8 +269,7 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
sctp_inq_free(&ep->base.inqueue);
sctp_bind_addr_free(&ep->base.bind_addr);
- for (i = 0; i < SCTP_HOW_MANY_SECRETS; ++i)
- memset(&ep->secret_key[i], 0, SCTP_SECRET_SIZE);
+ memset(ep->secret_key, 0, sizeof(ep->secret_key));
/* Remove and free the port */
if (sctp_sk(ep->base.sk)->bind_hash)
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index e1c5fc2..a193f3b 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1589,8 +1589,6 @@ static sctp_cookie_param_t *sctp_pack_cookie(const struct sctp_endpoint *ep,
struct sctp_signed_cookie *cookie;
struct scatterlist sg;
int headersize, bodysize;
- unsigned int keylen;
- char *key;
/* Header size is static data prior to the actual cookie, including
* any padding.
@@ -1650,12 +1648,11 @@ static sctp_cookie_param_t *sctp_pack_cookie(const struct sctp_endpoint *ep,
/* Sign the message. */
sg_init_one(&sg, &cookie->c, bodysize);
- keylen = SCTP_SECRET_SIZE;
- key = (char *)ep->secret_key[ep->current_key];
desc.tfm = sctp_sk(ep->base.sk)->hmac;
desc.flags = 0;
- if (crypto_hash_setkey(desc.tfm, key, keylen) ||
+ if (crypto_hash_setkey(desc.tfm, ep->secret_key,
+ sizeof(ep->secret_key)) ||
crypto_hash_digest(&desc, &sg, bodysize, cookie->signature))
goto free_cookie;
}
@@ -1682,8 +1679,7 @@ struct sctp_association *sctp_unpack_cookie(
int headersize, bodysize, fixed_size;
__u8 *digest = ep->digest;
struct scatterlist sg;
- unsigned int keylen, len;
- char *key;
+ unsigned int len;
sctp_scope_t scope;
struct sk_buff *skb = chunk->skb;
struct timeval tv;
@@ -1718,34 +1714,21 @@ struct sctp_association *sctp_unpack_cookie(
goto no_hmac;
/* Check the signature. */
- keylen = SCTP_SECRET_SIZE;
sg_init_one(&sg, bear_cookie, bodysize);
- key = (char *)ep->secret_key[ep->current_key];
desc.tfm = sctp_sk(ep->base.sk)->hmac;
desc.flags = 0;
memset(digest, 0x00, SCTP_SIGNATURE_SIZE);
- if (crypto_hash_setkey(desc.tfm, key, keylen) ||
+ if (crypto_hash_setkey(desc.tfm, ep->secret_key,
+ sizeof(ep->secret_key)) ||
crypto_hash_digest(&desc, &sg, bodysize, digest)) {
*error = -SCTP_IERROR_NOMEM;
goto fail;
}
if (memcmp(digest, cookie->signature, SCTP_SIGNATURE_SIZE)) {
- /* Try the previous key. */
- key = (char *)ep->secret_key[ep->last_key];
- memset(digest, 0x00, SCTP_SIGNATURE_SIZE);
- if (crypto_hash_setkey(desc.tfm, key, keylen) ||
- crypto_hash_digest(&desc, &sg, bodysize, digest)) {
- *error = -SCTP_IERROR_NOMEM;
- goto fail;
- }
-
- if (memcmp(digest, cookie->signature, SCTP_SIGNATURE_SIZE)) {
- /* Yikes! Still bad signature! */
- *error = -SCTP_IERROR_BAD_SIG;
- goto fail;
- }
+ *error = -SCTP_IERROR_BAD_SIG;
+ goto fail;
}
no_hmac:
--
1.7.11.7
^ permalink raw reply related
* Re: [PATCH 2/2] tcp: fix FIN_WAIT2 timer expression in /proc/net/tcp
From: Eric Dumazet @ 2013-02-12 15:13 UTC (permalink / raw)
To: Toshiaki Makita; +Cc: David S. Miller, netdev
In-Reply-To: <1360681070.13993.2.camel@edumazet-glaptop>
On Tue, 2013-02-12 at 06:57 -0800, Eric Dumazet wrote:
> I find this patch confusing :
>
> 1) Please don't change the indentation for a bug fix
>
> 2) You add a new 'active=3' field, that some user space
> reading /proc/net/tcp wont expect.
>
> So the changelog is not matching the changes.
Also, net/ipv4/inet_diag.c was not changed
^ permalink raw reply
* Re: team driver MAC NAT
From: Jamal Hadi Salim @ 2013-02-12 15:12 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, edumazet, bart.de.schuymer, stephen, pablo, kaber,
netfilter-devel
In-Reply-To: <20130212140247.GB18057@minipsycho.orion>
Jiri,
This would be a good fit for a tc action. This way such a feature can be
used by other netdevs (including plain bridge, and not just team)
The main question is how does the tc action learn about the MAC
addresses that
it needs to convert.
cheers,
jamal
On 13-02-12 09:02 AM, Jiri Pirko wrote:
> Hi all.
>
> I have following situation:
>
> host X ............. eth0 -> br0 <- team0 <- eth2 .............. switch ......... host Z
> host Y ............. eth1 ---^ ^---- eth3 .................^
>
> Now to achieve RX traffic balancing on team0, I have to mangle src mac of
> all outgoing packets. If the packet goes through eth2, src mac will be set to
> eth2 mac, same happens on eth3. I have to magle ARP packets as well.
>
> This would achieve something which might be called "MAC NAT".
>
> To unmangle RXed packets back to the original MAC adresses (of host X and host Y)
> I need to track "connections".
>
> Now the question is: Should this be integrated in netfilter infrastructure?
> I'm thinking something similar to existing conntrack. NF_HOOK will be placed
> in team driver rx/tx functions (similar to what bridge does).
>
> I think this would be nicer than to do it independently just in team driver code.
> Also this MAC NAT (with tracking connections, unlike what bridge MAC NAT does)
> can be probably handy for other purposes as well (not sure what)
>
> Thanks for any suggestions.
>
> Jiri
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ 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