* [PATCH net v2] bonding: fix div by zero while enslaving and transmitting
From: Nikolay Aleksandrov @ 2014-09-12 15:38 UTC (permalink / raw)
To: netdev
Cc: Nikolay Aleksandrov, Eric Dumazet, Andy Gospodarek, Jay Vosburgh,
Veaceslav Falico
In-Reply-To: <5413097B.9080802@redhat.com>
The problem is that the slave is first linked and slave_cnt is
incremented afterwards leading to a div by zero in the modes that use it
as a modulus. What happens is that in bond_start_xmit()
bond_has_slaves() is used to evaluate further transmission and it becomes
true after the slave is linked in, but when slave_cnt is used in the xmit
path it is still 0, so fetch it once and transmit based on that. Since
it is used only in round-robin and XOR modes, the fix is only for them.
Thanks to Eric Dumazet for pointing out the fault in my first try to fix
this.
Call trace (took it out of net-next kernel, but it's the same with net):
[46934.330038] divide error: 0000 [#1] SMP
[46934.330041] Modules linked in: bonding(O) 9p fscache
snd_hda_codec_generic crct10dif_pclmul
[46934.330041] bond0: Enslaving eth1 as an active interface with an up
link
[46934.330051] ppdev joydev crc32_pclmul crc32c_intel 9pnet_virtio
ghash_clmulni_intel snd_hda_intel 9pnet snd_hda_controller parport_pc
serio_raw pcspkr snd_hda_codec parport virtio_balloon virtio_console
snd_hwdep snd_pcm pvpanic i2c_piix4 snd_timer i2ccore snd soundcore
virtio_blk virtio_net virtio_pci virtio_ring virtio ata_generic
pata_acpi floppy [last unloaded: bonding]
[46934.330053] CPU: 1 PID: 3382 Comm: ping Tainted: G O
3.17.0-rc4+ #27
[46934.330053] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[46934.330054] task: ffff88005aebf2c0 ti: ffff88005b728000 task.ti:
ffff88005b728000
[46934.330059] RIP: 0010:[<ffffffffa0198c33>] [<ffffffffa0198c33>]
bond_start_xmit+0x1c3/0x450 [bonding]
[46934.330060] RSP: 0018:ffff88005b72b7f8 EFLAGS: 00010246
[46934.330060] RAX: 0000000000000679 RBX: ffff88004b077000 RCX:
000000000000002a
[46934.330061] RDX: 0000000000000000 RSI: ffff88004b3f0500 RDI:
ffff88004b077940
[46934.330061] RBP: ffff88005b72b830 R08: 00000000000000c0 R09:
ffff88004a83e000
[46934.330062] R10: 000000000000ffff R11: ffff88004b1f12c0 R12:
ffff88004b3f0500
[46934.330062] R13: ffff88004b3f0500 R14: 000000000000002a R15:
ffff88004b077940
[46934.330063] FS: 00007fbd91a4c740(0000) GS:ffff88005f080000(0000)
knlGS:0000000000000000
[46934.330064] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[46934.330064] CR2: 00007f803a8bb000 CR3: 000000004b2c9000 CR4:
00000000000406e0
[46934.330069] Stack:
[46934.330071] ffffffff811e6169 00000000e772fa05 ffff88004b077000
ffff88004b3f0500
[46934.330072] ffffffff81d17d18 000000000000002a 0000000000000000
ffff88005b72b8a0
[46934.330073] ffffffff81620108 ffffffff8161fe0e ffff88005b72b8c4
ffff88005b302000
[46934.330073] Call Trace:
[46934.330077] [<ffffffff811e6169>] ?
__kmalloc_node_track_caller+0x119/0x300
[46934.330084] [<ffffffff81620108>] dev_hard_start_xmit+0x188/0x410
[46934.330086] [<ffffffff8161fe0e>] ? harmonize_features+0x2e/0x90
[46934.330088] [<ffffffff81620b06>] __dev_queue_xmit+0x456/0x590
[46934.330089] [<ffffffff81620c50>] dev_queue_xmit+0x10/0x20
[46934.330090] [<ffffffff8168f022>] arp_xmit+0x22/0x60
[46934.330091] [<ffffffff8168f090>] arp_send.part.16+0x30/0x40
[46934.330092] [<ffffffff8168f1e5>] arp_solicit+0x115/0x2b0
[46934.330094] [<ffffffff8160b5d7>] ? copy_skb_header+0x17/0xa0
[46934.330096] [<ffffffff8162875a>] neigh_probe+0x4a/0x70
[46934.330097] [<ffffffff8162979c>] __neigh_event_send+0xac/0x230
[46934.330098] [<ffffffff8162a00b>] neigh_resolve_output+0x13b/0x220
[46934.330100] [<ffffffff8165f120>] ? ip_forward_options+0x1c0/0x1c0
[46934.330101] [<ffffffff81660478>] ip_finish_output+0x1f8/0x860
[46934.330102] [<ffffffff81661f08>] ip_output+0x58/0x90
[46934.330103] [<ffffffff81661602>] ? __ip_local_out+0xa2/0xb0
[46934.330104] [<ffffffff81661640>] ip_local_out_sk+0x30/0x40
[46934.330105] [<ffffffff81662a66>] ip_send_skb+0x16/0x50
[46934.330106] [<ffffffff81662ad3>] ip_push_pending_frames+0x33/0x40
[46934.330107] [<ffffffff8168854c>] raw_sendmsg+0x88c/0xa30
[46934.330110] [<ffffffff81612b31>] ? skb_recv_datagram+0x41/0x60
[46934.330111] [<ffffffff816875a9>] ? raw_recvmsg+0xa9/0x1f0
[46934.330113] [<ffffffff816978d4>] inet_sendmsg+0x74/0xc0
[46934.330114] [<ffffffff81697a9b>] ? inet_recvmsg+0x8b/0xb0
[46934.330115] bond0: Adding slave eth2
[46934.330116] [<ffffffff8160357c>] sock_sendmsg+0x9c/0xe0
[46934.330118] [<ffffffff81603248>] ?
move_addr_to_kernel.part.20+0x28/0x80
[46934.330121] [<ffffffff811b4477>] ? might_fault+0x47/0x50
[46934.330122] [<ffffffff816039b9>] ___sys_sendmsg+0x3a9/0x3c0
[46934.330125] [<ffffffff8144a14a>] ? n_tty_write+0x3aa/0x530
[46934.330127] [<ffffffff810d1ae4>] ? __wake_up+0x44/0x50
[46934.330129] [<ffffffff81242b38>] ? fsnotify+0x238/0x310
[46934.330130] [<ffffffff816048a1>] __sys_sendmsg+0x51/0x90
[46934.330131] [<ffffffff816048f2>] SyS_sendmsg+0x12/0x20
[46934.330134] [<ffffffff81738b29>] system_call_fastpath+0x16/0x1b
[46934.330144] Code: 48 8b 10 4c 89 ee 4c 89 ff e8 aa bc ff ff 31 c0 e9
1a ff ff ff 0f 1f 00 4c 89 ee 4c 89 ff e8 65 fb ff ff 31 d2 4c 89 ee 4c
89 ff <f7> b3 64 09 00 00 e8 02 bd ff ff 31 c0 e9 f2 fe ff ff 0f 1f 00
[46934.330146] RIP [<ffffffffa0198c33>] bond_start_xmit+0x1c3/0x450
[bonding]
[46934.330146] RSP <ffff88005b72b7f8>
CC: Eric Dumazet <eric.dumazet@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
Fixes: 278b208375 ("bonding: initial RCU conversion")
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: Based on Eric's feedback change the fix to fetch the value once in a
local variable in the affected modes and to act based on that.
I believe we don't need to move the increment now as it doesn't really
matter if it'll be before the wmb or after.
drivers/net/bonding/bond_main.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 57912ee231cb..798ae69fb63c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3659,8 +3659,14 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
else
bond_xmit_slave_id(bond, skb, 0);
} else {
- slave_id = bond_rr_gen_slave_id(bond);
- bond_xmit_slave_id(bond, skb, slave_id % bond->slave_cnt);
+ int slave_cnt = ACCESS_ONCE(bond->slave_cnt);
+
+ if (likely(slave_cnt)) {
+ slave_id = bond_rr_gen_slave_id(bond);
+ bond_xmit_slave_id(bond, skb, slave_id % slave_cnt);
+ } else {
+ dev_kfree_skb_any(skb);
+ }
}
return NETDEV_TX_OK;
@@ -3691,8 +3697,13 @@ static int bond_xmit_activebackup(struct sk_buff *skb, struct net_device *bond_d
static int bond_xmit_xor(struct sk_buff *skb, struct net_device *bond_dev)
{
struct bonding *bond = netdev_priv(bond_dev);
+ int slave_cnt = ACCESS_ONCE(bond->slave_cnt);
- bond_xmit_slave_id(bond, skb, bond_xmit_hash(bond, skb) % bond->slave_cnt);
+ if (likely(slave_cnt))
+ bond_xmit_slave_id(bond, skb,
+ bond_xmit_hash(bond, skb) % slave_cnt);
+ else
+ dev_kfree_skb_any(skb);
return NETDEV_TX_OK;
}
--
1.9.3
^ permalink raw reply related
* Re: PTP_PEROUT_REQUEST and clock stepping
From: Daniel Glöckner @ 2014-09-12 15:19 UTC (permalink / raw)
To: Richard Cochran; +Cc: Christian Riesch, netdev@vger.kernel.org
In-Reply-To: <20140912144025.GA20712@netboy>
On 09/12/14 16:40, Richard Cochran wrote:
>> Taking the Intel i210 as an example, the third option (doing nothing)
>> would lead to the following behavior:
>>
>> - If the clock is stepped back, the periodic output stops until the
>> clock reaches the point when it was adjusted.
>>
>> - If the clock is stepped forward, the periodic output oscillates at
>> 62,5 MHz to catch up.
>
> That is a good example of "the results are undefined".
>
> So, how would you reprogram the i210 to keep the period output
> continuous?
0. I'm assuming here that new_time >= start
1. stop the periodic output via TSAUXC
2. set the clock via SYSTIM* to new_time
3a. if (new_time - start) % period < period / 2
set the TRGTTIM* regs to new_time - (new_time - start) % period
3b. else
set the TRGTTIM* regs to new_time - (new_time - start) % period
+ period
4. start the periodic output via TSAUXC
Best Regards,
Daniel
--
Dipl.-Math. Daniel Glöckner, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11,
Bertha-von-Suttner-Straße 9, 37085 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführer: Dr. Uwe Kracke, Ust-IdNr.: DE 205 198 055
emlix - your embedded linux partner
^ permalink raw reply
* Re: [net-next PATCH v4 12/16] net: sched: rcu'ify cls_bpf
From: John Fastabend @ 2014-09-12 15:16 UTC (permalink / raw)
To: Eric Dumazet; +Cc: xiyou.wangcong, davem, jhs, netdev, paulmck, brouer
In-Reply-To: <1410402481.7106.51.camel@edumazet-glaptop2.roam.corp.google.com>
On 09/10/2014 07:28 PM, Eric Dumazet wrote:
> On Wed, 2014-09-10 at 08:51 -0700, John Fastabend wrote:
>> This patch makes the cls_bpf classifier RCU safe. The tcf_lock
>> was being used to protect a list of cls_bpf_prog now this list
>> is RCU safe and updates occur with rcu_replace.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>
> ...
[...]
>> @@ -281,15 +280,17 @@ static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
>> if (ret < 0)
>> goto errout;
>>
>> - tcf_tree_lock(tp);
>> - list_add(&prog->link, &head->plist);
>> - tcf_tree_unlock(tp);
>> + if (oldprog) {
>> + list_replace_rcu(&prog->link, &oldprog->link);
>> + call_rcu(&oldprog->rcu, __cls_bpf_delete_prog);
>> + } else {
>> + list_add_rcu(&prog->link, &head->plist);
>> + }
>>
>> *arg = (unsigned long) prog;
>> -
>> return 0;
>> errout:
>> - if (*arg == 0UL && prog)
>> + if (prog)
>> kfree(prog);
>>
>
> nit, you can directly call kfree(prog) even if prog == NULL
>
Yep, made the change.
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [net-next PATCH v4 11/16] net: sched: rcu'ify cls_rsvp
From: John Fastabend @ 2014-09-12 15:13 UTC (permalink / raw)
To: Eric Dumazet; +Cc: xiyou.wangcong, davem, jhs, netdev, paulmck, brouer
In-Reply-To: <1410399058.7106.50.camel@edumazet-glaptop2.roam.corp.google.com>
On 09/10/2014 06:30 PM, Eric Dumazet wrote:
> On Wed, 2014-09-10 at 08:51 -0700, John Fastabend wrote:
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
>> net/sched/cls_rsvp.h | 157 ++++++++++++++++++++++++++++----------------------
>> 1 file changed, 89 insertions(+), 68 deletions(-)
>>
>> diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
>> index 1020e23..afa508b 100644
>> --- a/net/sched/cls_rsvp.h
>> +++ b/net/sched/cls_rsvp.h
>> @@ -70,31 +70,34 @@ struct rsvp_head {
>> u32 tmap[256/32];
>> u32 hgenerator;
>> u8 tgenerator;
>> - struct rsvp_session *ht[256];
>> + struct rsvp_session __rcu *ht[256];
>> + struct rcu_head rcu;
>> };
>>
>> struct rsvp_session {
>> - struct rsvp_session *next;
>> - __be32 dst[RSVP_DST_LEN];
>> - struct tc_rsvp_gpi dpi;
>> - u8 protocol;
>> - u8 tunnelid;
>> + struct rsvp_session __rcu *next;
>> + __be32 dst[RSVP_DST_LEN];
>> + struct tc_rsvp_gpi dpi;
>> + u8 protocol;
>> + u8 tunnelid;
>> /* 16 (src,sport) hash slots, and one wildcard source slot */
>> - struct rsvp_filter *ht[16 + 1];
>> + struct rsvp_filter __rcu *ht[16 + 1];
>> + struct rcu_head rcu;
>> };
>>
>>
>> struct rsvp_filter {
>> - struct rsvp_filter *next;
>> - __be32 src[RSVP_DST_LEN];
>> - struct tc_rsvp_gpi spi;
>> - u8 tunnelhdr;
>> + struct rsvp_filter __rcu *next;
>> + __be32 src[RSVP_DST_LEN];
>> + struct tc_rsvp_gpi spi;
>> + u8 tunnelhdr;
>>
>> - struct tcf_result res;
>> - struct tcf_exts exts;
>> + struct tcf_result res;
>> + struct tcf_exts exts;
>>
>> - u32 handle;
>> - struct rsvp_session *sess;
>> + u32 handle;
>> + struct rsvp_session *sess;
>> + struct rcu_head rcu;
>> };
>>
>> static inline unsigned int hash_dst(__be32 *dst, u8 protocol, u8 tunnelid)
>> @@ -128,7 +131,7 @@ static inline unsigned int hash_src(__be32 *src)
>> static int rsvp_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>> struct tcf_result *res)
>> {
>> - struct rsvp_session **sht = ((struct rsvp_head *)tp->root)->ht;
>> + struct rsvp_head *head = rcu_dereference_bh(tp->root);
>> struct rsvp_session *s;
>> struct rsvp_filter *f;
>> unsigned int h1, h2;
>> @@ -169,7 +172,8 @@ restart:
>> h1 = hash_dst(dst, protocol, tunnelid);
>> h2 = hash_src(src);
>>
>> - for (s = sht[h1]; s; s = s->next) {
>> + for (s = rcu_dereference_bh(head->ht[h1]); s;
>> + s = rcu_dereference_bh(s->next)) {
>> if (dst[RSVP_DST_LEN-1] == s->dst[RSVP_DST_LEN - 1] &&
>> protocol == s->protocol &&
>> !(s->dpi.mask &
>> @@ -181,7 +185,8 @@ restart:
>> #endif
>> tunnelid == s->tunnelid) {
>>
>> - for (f = s->ht[h2]; f; f = f->next) {
>> + for (f = rcu_dereference_bh(s->ht[h2]); f;
>> + f = rcu_dereference_bh(f->next)) {
>> if (src[RSVP_DST_LEN-1] == f->src[RSVP_DST_LEN - 1] &&
>> !(f->spi.mask & (*(u32 *)(xprt + f->spi.offset) ^ f->spi.key))
>> #if RSVP_DST_LEN == 4
>> @@ -205,7 +210,8 @@ matched:
>> }
>>
>> /* And wildcard bucket... */
>> - for (f = s->ht[16]; f; f = f->next) {
>> + for (f = rcu_dereference_bh(s->ht[16]); f;
>> + f = rcu_dereference_bh(f->next)) {
>> *res = f->res;
>> RSVP_APPLY_RESULT();
>> goto matched;
>> @@ -218,7 +224,7 @@ matched:
>>
>> static unsigned long rsvp_get(struct tcf_proto *tp, u32 handle)
>> {
>> - struct rsvp_session **sht = ((struct rsvp_head *)tp->root)->ht;
>> + struct rsvp_head *head = rtnl_dereference(tp->root);
>> struct rsvp_session *s;
>> struct rsvp_filter *f;
>> unsigned int h1 = handle & 0xFF;
>> @@ -227,8 +233,10 @@ static unsigned long rsvp_get(struct tcf_proto *tp, u32 handle)
>> if (h2 > 16)
>> return 0;
>>
>> - for (s = sht[h1]; s; s = s->next) {
>> - for (f = s->ht[h2]; f; f = f->next) {
>> + for (s = rtnl_dereference(head->ht[h1]); s;
>> + s = rtnl_dereference(s->next)) {
>> + for (f = rtnl_dereference(s->ht[h2]); f;
>> + f = rtnl_dereference(f->next)) {
>> if (f->handle == handle)
>> return (unsigned long)f;
>> }
>> @@ -246,7 +254,7 @@ static int rsvp_init(struct tcf_proto *tp)
>>
>> data = kzalloc(sizeof(struct rsvp_head), GFP_KERNEL);
>> if (data) {
>> - tp->root = data;
>> + rcu_assign_pointer(tp->root, data);
>> return 0;
>> }
>> return -ENOBUFS;
>> @@ -257,53 +265,55 @@ rsvp_delete_filter(struct tcf_proto *tp, struct rsvp_filter *f)
>> {
>> tcf_unbind_filter(tp, &f->res);
>> tcf_exts_destroy(tp, &f->exts);
>> - kfree(f);
>> + kfree_rcu(f, rcu);
>> }
>>
>> static void rsvp_destroy(struct tcf_proto *tp)
>> {
>> - struct rsvp_head *data = xchg(&tp->root, NULL);
>> - struct rsvp_session **sht;
>> + struct rsvp_head *data = rtnl_dereference(tp->root);
>> int h1, h2;
>>
>> if (data == NULL)
>> return;
>>
>> - sht = data->ht;
>> + RCU_INIT_POINTER(tp->root, NULL);
>>
>> for (h1 = 0; h1 < 256; h1++) {
>> struct rsvp_session *s;
>>
>> - while ((s = sht[h1]) != NULL) {
>> - sht[h1] = s->next;
>> + while ((s = rtnl_dereference(data->ht[h1])) != NULL) {
>> + RCU_INIT_POINTER(data->ht[h1], s->next);
>>
>> for (h2 = 0; h2 <= 16; h2++) {
>> struct rsvp_filter *f;
>>
>> - while ((f = s->ht[h2]) != NULL) {
>> - s->ht[h2] = f->next;
>> + while ((f = rtnl_dereference(s->ht[h2])) != NULL) {
>> + rcu_assign_pointer(s->ht[h2], f->next);
>> rsvp_delete_filter(tp, f);
>> }
>> }
>> - kfree(s);
>> + kfree_rcu(s, rcu);
>> }
>> }
>> - kfree(data);
>> + RCU_INIT_POINTER(tp->root, NULL);
>
> Strange, you already did the RCU_INIT_POINTER(tp->root, NULL) before the
> for(h1 = 0; h1 < 256; h1++) loop
>
Yep, I'll drop this duplicate call.
>
>
>> + kfree_rcu(data, rcu);
>> }
>>
>> static int rsvp_delete(struct tcf_proto *tp, unsigned long arg)
>> {
>> - struct rsvp_filter **fp, *f = (struct rsvp_filter *)arg;
>> + struct rsvp_head *head = rtnl_dereference(tp->root);
>> + struct rsvp_filter *nfp, *f = (struct rsvp_filter *)arg;
>> + struct rsvp_filter __rcu **fp;
>> unsigned int h = f->handle;
>> - struct rsvp_session **sp;
>> - struct rsvp_session *s = f->sess;
>> + struct rsvp_session __rcu **sp;
>> + struct rsvp_session *nsp, *s = f->sess;
>> int i;
>>
>> - for (fp = &s->ht[(h >> 8) & 0xFF]; *fp; fp = &(*fp)->next) {
>> - if (*fp == f) {
>> - tcf_tree_lock(tp);
>> + fp = &s->ht[(h >> 8) & 0xFF];
>> + for (nfp = rtnl_dereference(*fp); nfp;
>> + fp = &nfp->next, nfp = rtnl_dereference(*fp)) {
>> + if (nfp == f) {
>> *fp = f->next;
>
> It seems you do not follow your own convention here ?
>
> RCU_INIT_POINTER(*fp, f->next);
converted both cases to RCU_INIT_POINTER().
Thanks.
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [net-next PATCH v4 02/16] net: rcu-ify tcf_proto
From: John Fastabend @ 2014-09-12 15:03 UTC (permalink / raw)
To: Eric Dumazet; +Cc: xiyou.wangcong, davem, jhs, netdev, paulmck, brouer
In-Reply-To: <1410397004.7106.41.camel@edumazet-glaptop2.roam.corp.google.com>
On 09/10/2014 05:56 PM, Eric Dumazet wrote:
> On Wed, 2014-09-10 at 08:47 -0700, John Fastabend wrote:
>> rcu'ify tcf_proto this allows calling tc_classify() without holding
>> any locks. Updaters are protected by RTNL.
>>
>> This patch prepares the core net_sched infrastracture for running
>> the classifier/action chains without holding the qdisc lock however
>> it does nothing to ensure cls_xxx and act_xxx types also work without
>> locking. Additional patches are required to address the fall out.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> ---
> ...
>> diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
>> index ed30e43..4b52b70 100644
>> --- a/net/sched/sch_choke.c
>> +++ b/net/sched/sch_choke.c
>> @@ -57,7 +57,7 @@ struct choke_sched_data {
>>
>> /* Variables */
>> struct red_vars vars;
>> - struct tcf_proto *filter_list;
>> + struct tcf_proto __rcu *filter_list;
>> struct {
>> u32 prob_drop; /* Early probability drops */
>> u32 prob_mark; /* Early probability marks */
>> @@ -193,9 +193,11 @@ static bool choke_classify(struct sk_buff *skb,
>> {
>> struct choke_sched_data *q = qdisc_priv(sch);
>> struct tcf_result res;
>> + struct tcf_proto *fl;
>> int result;
>>
>> - result = tc_classify(skb, q->filter_list, &res);
>> + fl = rcu_dereference_bh(q->filter_list);
>
> Hmm... please change the caller to pass fl.
>
> Idea is to read q->filter_list once.
>
I'll just use rcu_access_pointer() in the caller and leave this
rcu_dereference_bh() here.
>> + result = tc_classify(skb, fl, &res);
>> if (result >= 0) {
>> #ifdef CONFIG_NET_CLS_ACT
>> switch (result) {
>> @@ -244,12 +246,14 @@ static bool choke_match_random(const struct choke_sched_data *q,
>> unsigned int *pidx)
>> {
>> struct sk_buff *oskb;
>> + struct tcf_proto *fl;
>>
>> if (q->head == q->tail)
>> return false;
>>
>> oskb = choke_peek_random(q, pidx);
>> - if (q->filter_list)
>> + fl = rcu_dereference_bh(q->filter_list);
>
> You could use rcu_access_pointer() and not have this fl variable.
>
done thanks.
>> + if (fl)
>> return choke_get_classid(nskb) == choke_get_classid(oskb);
>>
>> return choke_match_flow(oskb, nskb);
>> @@ -259,9 +263,11 @@ static int choke_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>> {
>> struct choke_sched_data *q = qdisc_priv(sch);
>> const struct red_parms *p = &q->parms;
>> + struct tcf_proto *fl;
>> int ret = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
>>
>> - if (q->filter_list) {
>> + fl = rcu_dereference_bh(q->filter_list);
>> + if (fl) {
>> /* If using external classifiers, get result and record it. */
>> if (!choke_classify(skb, sch, &ret))
>
> Here I think you should pass fl as an additional parameter to
> choke_classify()
>
>
> OR, just use rcu_access_pointer() here as you do not deref
> q->filter_list here.
>
Went with rcu_access_pointer.
>
>> goto other_drop; /* Packet was eaten by filter */
>> @@ -554,7 +560,8 @@ static unsigned long choke_bind(struct Qdisc *sch, unsigned long parent,
>> return 0;
>> }
>>
>> -static struct tcf_proto **choke_find_tcf(struct Qdisc *sch, unsigned long cl)
>> +static struct tcf_proto __rcu **choke_find_tcf(struct Qdisc *sch,
>> + unsigned long cl)
>> {
>> struct choke_sched_data *q = qdisc_priv(sch);
>>
>
> remaining part seems fine.
>
>
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [PATCH net] bonding: fix div by zero while enslaving and transmitting
From: Nikolay Aleksandrov @ 2014-09-12 14:55 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Jiri Pirko, Andy Gospodarek, Jay Vosburgh,
Veaceslav Falico
In-Reply-To: <1410533152.7106.95.camel@edumazet-glaptop2.roam.corp.google.com>
On 09/12/2014 04:45 PM, Eric Dumazet wrote:
> On Fri, 2014-09-12 at 15:33 +0200, Nikolay Aleksandrov wrote:
>
>> One more thing, netdev_master_upper_dev_link_private() which is called
>> after the increment uses list_add_rcu() (i.e. rcu_assign_pointer) to insert
>> the slave, so there's a barrier there to ensure this is visible before the
>> slave is linked.
>
> You missed my point.
>
> You fixed the writer side, without adding barriers on the read side.
>
> Without looking at the code, just reading your patch, I spot a problem.
>
> Following code is fundamentally broken :
>
> rcu_read_lock();
>
> if (bond->list) {
> x = y % bond->slave; // bug : cpu could fetch bond->slave before bond->list
>
> rcu_read_unlock();
>
>
Ah yes, you're absolutely correct.
Then get the value of slave_cnt in a local variable using ACCESS_ONCE() + a
check for != 0 would be a better solution than adding a read barrier, what
do you think ?
Since it's not a problem to see slave_cnt > 0 when there're no slaves, but
the other way around is problematic.
> Because it needs a read barrier, since the writer does after your patch:
>
> update bond->slave
> smp_wmb();
> update bond->list
>
> I repeat : adding few rcu_read_lock() / rcu_read_unlock() /
> rcu_assign_pointer() calls is not enough to guarantee code is not
> broken.
>
Yes, I see now.
Thanks,
Nik
>
>
> --
> 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
* Re: [PATCH net] bonding: fix div by zero while enslaving and transmitting
From: Eric Dumazet @ 2014-09-12 14:45 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, Jiri Pirko, Andy Gospodarek, Jay Vosburgh,
Veaceslav Falico
In-Reply-To: <5412F643.10007@redhat.com>
On Fri, 2014-09-12 at 15:33 +0200, Nikolay Aleksandrov wrote:
> One more thing, netdev_master_upper_dev_link_private() which is called
> after the increment uses list_add_rcu() (i.e. rcu_assign_pointer) to insert
> the slave, so there's a barrier there to ensure this is visible before the
> slave is linked.
You missed my point.
You fixed the writer side, without adding barriers on the read side.
Without looking at the code, just reading your patch, I spot a problem.
Following code is fundamentally broken :
rcu_read_lock();
if (bond->list) {
x = y % bond->slave; // bug : cpu could fetch bond->slave before bond->list
rcu_read_unlock();
Because it needs a read barrier, since the writer does after your patch:
update bond->slave
smp_wmb();
update bond->list
I repeat : adding few rcu_read_lock() / rcu_read_unlock() /
rcu_assign_pointer() calls is not enough to guarantee code is not
broken.
^ permalink raw reply
* Re: PTP_PEROUT_REQUEST and clock stepping
From: Richard Cochran @ 2014-09-12 14:40 UTC (permalink / raw)
To: Daniel Glöckner; +Cc: Christian Riesch, netdev@vger.kernel.org
In-Reply-To: <5412DBB0.9010008@emlix.com>
On Fri, Sep 12, 2014 at 01:40:32PM +0200, Daniel Glöckner wrote:
> You mean the hardware that you know of that currently exists.
Yes, that is the hardware we support. I am sorry that I don't know how
to support mythical hardware.
> I just think it is wrong to place the burden of stopping and starting
> the periodic output on the PTP demon.
I think it is the right place.
> The kernel knows when clock_adjtime
> and clock_settime are called and it also knows which periodic outputs
> are enabled and how they are configured. We just have to decide on the
> correct behavior (respect .start vs. ignore .start once running) to be
> implemented.
The only reasonable actions for kernel would be to either stop all
periodic outputs, or to just do nothing.
> Taking the Intel i210 as an example, the third option (doing nothing)
> would lead to the following behavior:
>
> - If the clock is stepped back, the periodic output stops until the
> clock reaches the point when it was adjusted.
>
> - If the clock is stepped forward, the periodic output oscillates at
> 62,5 MHz to catch up.
That is a good example of "the results are undefined".
So, how would you reprogram the i210 to keep the period output
continuous?
Thanks,
Richard
^ permalink raw reply
* [RFC] ipv4: Do not cache routing failures due to disabled forwarding.
From: Nicolas Cavallari @ 2014-09-12 14:14 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
In-Reply-To: <1410531260-13794-1-git-send-email-nicolas.cavallari@green-communications.fr>
If we cache them, the kernel will reuse them, independently of
whether forwarding is enabled or not. Which means that if forwarding is
disabled on the input interface where the first routing request comes
from, then that unreachable result will be cached and reused for
other interfaces, even if forwarding is enabled on them.
This can be verified with two interfaces A and B and an output interface
C, where B has forwarding enabled, but not A and trying
ip route get $dst iif A from $src && ip route get $dst iif B from $src
Signed-off-by: Nicolas Cavallari <nicolas.cavallari@green-communications.fr>
---
based on net-next, but not really tested on top of it.
net/ipv4/route.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 234a43e..b537997 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1655,7 +1655,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
struct rtable *rth;
int err = -EINVAL;
struct net *net = dev_net(dev);
- bool do_cache;
+ bool do_cache = true;
/* IP on this device is disabled. */
@@ -1723,6 +1723,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
if (!IN_DEV_FORWARD(in_dev)) {
err = -EHOSTUNREACH;
+ do_cache = false;
goto no_route;
}
if (res.type != RTN_UNICAST)
@@ -1746,16 +1747,14 @@ brd_input:
RT_CACHE_STAT_INC(in_brd);
local_input:
- do_cache = false;
- if (res.fi) {
- if (!itag) {
- rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input);
- if (rt_cache_valid(rth)) {
- skb_dst_set_noref(skb, &rth->dst);
- err = 0;
- goto out;
- }
- do_cache = true;
+ if (!res.fi || itag) {
+ do_cache = false;
+ } else if (do_cache) {
+ rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input);
+ if (rt_cache_valid(rth)) {
+ skb_dst_set_noref(skb, &rth->dst);
+ err = 0;
+ goto out;
}
}
--
2.1.0
^ permalink raw reply related
* About caching unreachable routes when not forwarding
From: Nicolas Cavallari @ 2014-09-12 14:14 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
I have some weird routing problem on a seemingly simple setup on a
3.12-3.16 kernel and I suspect that net-next is also affected.
I have two interfaces: A, B, with forwarding disabled on A and
enabled on B. I also have another interface V.
Both interfaces receive a packet that must be routed though V:
- A receives the packet first, do some fib lookup and cache a "route
unreachable" rt_iif=0 because forwarding is disabled.
- B receives the packet, do some fib lookup (and reverse path filtering, whatever) and succeeds, then finds the recently cached rth and use it. This cached rth is of course "route unreachable" and forwarding is broken.
This simple script in a network namespace is enough to show it:
#!/bin/sh -eu
setup_iface () {
ip link add name "$1" type dummy
ip link set dev "$1" up
[ -n "${2:-}" ] && ip route add "$2" dev "$1"
ip4conf="/proc/sys/net/ipv4/conf/$1"
echo "$3" > "$ip4conf/forwarding"
echo "$4" > "$ip4conf/rp_filter"
}
ip link set lo up
setup_iface "A" "" 0 0
setup_iface "B" "10.0.0.2/32" 1 1
setup_iface "C" "10.0.0.1/32" 1 0
set -x +e
ip route get 10.0.0.1 from 10.0.0.2 iif A
# unreachable
ip route get 10.0.0.1 from 10.0.0.2 iif B
# unreachable, but should be reachable
ip route flush cache
ip route get 10.0.0.1 from 10.0.0.2 iif B
# reachable
ip route get 10.0.0.1 from 10.0.0.2 iif A
# reachable, but should be unreachable
I would suggest that we shouldn't cache unreachable results due to
forwarding being disabled, but I'm not aware of all use-case of this
code nor what should actually be cached. The following patch fix
my use case. I don't know if it breaks others.
^ permalink raw reply
* Re: [PATCH net] bonding: fix div by zero while enslaving and transmitting
From: Nikolay Aleksandrov @ 2014-09-12 13:33 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Jiri Pirko, Andy Gospodarek, Jay Vosburgh,
Veaceslav Falico
In-Reply-To: <5412F4D5.3070101@redhat.com>
On 09/12/2014 03:27 PM, Nikolay Aleksandrov wrote:
> On 09/12/2014 03:09 PM, Eric Dumazet wrote:
>> On Fri, 2014-09-12 at 14:22 +0200, Nikolay Aleksandrov wrote:
>> ...
>>>
>>> CC: Jiri Pirko <jiri@resnulli.us>
>>> CC: Andy Gospodarek <andy@greyhouse.net>
>>> CC: Jay Vosburgh <j.vosburgh@gmail.com>
>>> CC: Veaceslav Falico <vfalico@gmail.com>
>>> Fixes: 5378c2e6ea236d ("bonding: move bond-specific init after enslave happens")
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>>> ---
>>> drivers/net/bonding/bond_main.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 57912ee231cb..10ad434ea184 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1552,6 +1552,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>> goto err_detach;
>>> }
>>>
>>> + /* Increment slave_cnt before linking in the slave so we won't end up in
>>> + * bond_start_xmit with bond_has_slaves() true and slave_cnt == 0.
>>> + */
>>> + bond->slave_cnt++;
>>
>> It looks like explicit barriers are missing.
>>
One more thing, netdev_master_upper_dev_link_private() which is called
after the increment uses list_add_rcu() (i.e. rcu_assign_pointer) to insert
the slave, so there's a barrier there to ensure this is visible before the
slave is linked.
>> #define bond_has_slaves(bond) !list_empty(bond_slave_list(bond))
>>
>> So your increment into slave_cnt must be committed into memory before
>> any change to slave_list. But you need to check how removal of a slave
>> is handled.
>>
> That is handled by decrementing slave_cnt after executing synchronize_rcu()
> after unlinking the last slave thus making the list empty and all xmitters
> entering will see bond_has_slaves() as empty before they see slave_cnt as 0.
> In every other case the worst that could happen is that a few packets will
> see wrong slave_cnt, but that is not a problem since we walk the list to
> find the slave with the id.
>
>> Now I wonder why bond_has_slaves(bond) is not a test against
>> bond->slave_cnt
>>
> It used to be once, I don't remember the reason it's not anymore.
>
>> Note that even if this would be the case, bond xmit seems racy :
>>
>> if (bond_has_slaves(bond))
>> ret = __bond_start_xmit(skb, dev);
>>
> Yes, true but we make sure it doesn't see slave_cnt as 0 with
> bond_has_slaves() evaluating to true.
>
>> As slave_cnt could change (and eventually reach 0) between the two
>> places.
> This shouldn't be possible because of the synchronize_rcu() after unlinking
> the slave. slave_cnt is decremented only after that so every reader will
> see the list empty before they see slave_cnt as 0.
>
>>
>> My feeling is that RCU conversion is not properly done in this driver.
>>
>> Either bond->slave_cnt should be read _once_ for the whole duration of
>> bond_start_xmit() call, _OR_, be stored in a real Read Copy structure,
>> so that struct->slave_cnt _cannot_ change during bond_start_xmit()
>>
>>> res = bond_master_upper_dev_link(bond_dev, slave_dev, new_slave);
>>> if (res) {
>>> netdev_dbg(bond_dev, "Error %d calling bond_master_upper_dev_link\n", res);
>>> @@ -1564,7 +1568,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>> goto err_upper_unlink;
>>> }
>>>
>>> - bond->slave_cnt++;
>>> bond_compute_features(bond);
>>> bond_set_carrier(bond);
>>>
>>> @@ -1590,6 +1593,7 @@ err_upper_unlink:
>>>
>>> err_unregister:
>>> netdev_rx_handler_unregister(slave_dev);
>>> + bond->slave_cnt--;
>>>
>>> err_detach:
>>> if (!bond_uses_primary(bond))
>>
>>
>
> --
> 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
* Re: [PATCH net] net: fix sysfs symlinks of adjacent devices
From: Vlad Yasevich @ 2014-09-12 13:33 UTC (permalink / raw)
To: Alexander Fomichev, netdev
Cc: David Miller, Cong Wang, Vlad Yasevich, Andres Freund
In-Reply-To: <1410516826-6926-1-git-send-email-git.user@gmail.com>
On 09/12/2014 06:13 AM, Alexander Fomichev wrote:
> From: "Alexander Y. Fomichev" <git.user@gmail.com>
>
> __netdev_adjacent_dev_insert may add adjacent device from another
> namespace. Without proper check it leads to emergence of broken
> symlink from/to device not existing in current namespace.
> Fix: check net_ns is the same before netdev_adjacent_sysfs_add/del
> related to: 4c75431ac3520631f1d9e74aa88407e6374dbbc4
>
> Signed-off-by: Alexander Y. Fomichev <git.user@gmail.com>
> ---
> net/core/dev.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ab9a16530c36..887784b2dcde 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4841,7 +4841,8 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,
> pr_debug("dev_hold for %s, because of link added from %s to %s\n",
> adj_dev->name, dev->name, adj_dev->name);
>
> - if (netdev_adjacent_is_neigh_list(dev, dev_list)) {
> + if (netdev_adjacent_is_neigh_list(dev, dev_list) &&
> + net_eq(dev_net(dev), dev_net(adj_dev))) {
> ret = netdev_adjacent_sysfs_add(dev, adj_dev, dev_list);
> if (ret)
> goto free_adj;
> @@ -4862,7 +4863,8 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,
> return 0;
>
> remove_symlinks:
> - if (netdev_adjacent_is_neigh_list(dev, dev_list))
> + if (netdev_adjacent_is_neigh_list(dev, dev_list) &&
> + net_eq(dev_net(dev), dev_net(adj_dev)))
> netdev_adjacent_sysfs_del(dev, adj_dev->name, dev_list);
> free_adj:
> kfree(adj);
>
Looking over the code, it might make sense to move all the net_eq checks
into adjacent_sysfs calls so as to consolidate them. I haven't audited
all code paths, but at first glance it should do the right thing.
What do you think?
-vlad
^ permalink raw reply
* Re: [PATCH net] bonding: fix div by zero while enslaving and transmitting
From: Nikolay Aleksandrov @ 2014-09-12 13:27 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Jiri Pirko, Andy Gospodarek, Jay Vosburgh,
Veaceslav Falico
In-Reply-To: <1410527381.7106.81.camel@edumazet-glaptop2.roam.corp.google.com>
On 09/12/2014 03:09 PM, Eric Dumazet wrote:
> On Fri, 2014-09-12 at 14:22 +0200, Nikolay Aleksandrov wrote:
> ...
>>
>> CC: Jiri Pirko <jiri@resnulli.us>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> CC: Jay Vosburgh <j.vosburgh@gmail.com>
>> CC: Veaceslav Falico <vfalico@gmail.com>
>> Fixes: 5378c2e6ea236d ("bonding: move bond-specific init after enslave happens")
>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>> ---
>> drivers/net/bonding/bond_main.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 57912ee231cb..10ad434ea184 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1552,6 +1552,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> goto err_detach;
>> }
>>
>> + /* Increment slave_cnt before linking in the slave so we won't end up in
>> + * bond_start_xmit with bond_has_slaves() true and slave_cnt == 0.
>> + */
>> + bond->slave_cnt++;
>
> It looks like explicit barriers are missing.
>
> #define bond_has_slaves(bond) !list_empty(bond_slave_list(bond))
>
> So your increment into slave_cnt must be committed into memory before
> any change to slave_list. But you need to check how removal of a slave
> is handled.
>
That is handled by decrementing slave_cnt after executing synchronize_rcu()
after unlinking the last slave thus making the list empty and all xmitters
entering will see bond_has_slaves() as empty before they see slave_cnt as 0.
In every other case the worst that could happen is that a few packets will
see wrong slave_cnt, but that is not a problem since we walk the list to
find the slave with the id.
> Now I wonder why bond_has_slaves(bond) is not a test against
> bond->slave_cnt
>
It used to be once, I don't remember the reason it's not anymore.
> Note that even if this would be the case, bond xmit seems racy :
>
> if (bond_has_slaves(bond))
> ret = __bond_start_xmit(skb, dev);
>
Yes, true but we make sure it doesn't see slave_cnt as 0 with
bond_has_slaves() evaluating to true.
> As slave_cnt could change (and eventually reach 0) between the two
> places.
This shouldn't be possible because of the synchronize_rcu() after unlinking
the slave. slave_cnt is decremented only after that so every reader will
see the list empty before they see slave_cnt as 0.
>
> My feeling is that RCU conversion is not properly done in this driver.
>
> Either bond->slave_cnt should be read _once_ for the whole duration of
> bond_start_xmit() call, _OR_, be stored in a real Read Copy structure,
> so that struct->slave_cnt _cannot_ change during bond_start_xmit()
>
>> res = bond_master_upper_dev_link(bond_dev, slave_dev, new_slave);
>> if (res) {
>> netdev_dbg(bond_dev, "Error %d calling bond_master_upper_dev_link\n", res);
>> @@ -1564,7 +1568,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> goto err_upper_unlink;
>> }
>>
>> - bond->slave_cnt++;
>> bond_compute_features(bond);
>> bond_set_carrier(bond);
>>
>> @@ -1590,6 +1593,7 @@ err_upper_unlink:
>>
>> err_unregister:
>> netdev_rx_handler_unregister(slave_dev);
>> + bond->slave_cnt--;
>>
>> err_detach:
>> if (!bond_uses_primary(bond))
>
>
^ permalink raw reply
* Re: [PATCH net] bonding: fix div by zero while enslaving and transmitting
From: Eric Dumazet @ 2014-09-12 13:09 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, Jiri Pirko, Andy Gospodarek, Jay Vosburgh,
Veaceslav Falico
In-Reply-To: <1410524556-27128-1-git-send-email-nikolay@redhat.com>
On Fri, 2014-09-12 at 14:22 +0200, Nikolay Aleksandrov wrote:
...
>
> CC: Jiri Pirko <jiri@resnulli.us>
> CC: Andy Gospodarek <andy@greyhouse.net>
> CC: Jay Vosburgh <j.vosburgh@gmail.com>
> CC: Veaceslav Falico <vfalico@gmail.com>
> Fixes: 5378c2e6ea236d ("bonding: move bond-specific init after enslave happens")
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
> drivers/net/bonding/bond_main.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 57912ee231cb..10ad434ea184 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1552,6 +1552,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> goto err_detach;
> }
>
> + /* Increment slave_cnt before linking in the slave so we won't end up in
> + * bond_start_xmit with bond_has_slaves() true and slave_cnt == 0.
> + */
> + bond->slave_cnt++;
It looks like explicit barriers are missing.
#define bond_has_slaves(bond) !list_empty(bond_slave_list(bond))
So your increment into slave_cnt must be committed into memory before
any change to slave_list. But you need to check how removal of a slave
is handled.
Now I wonder why bond_has_slaves(bond) is not a test against
bond->slave_cnt
Note that even if this would be the case, bond xmit seems racy :
if (bond_has_slaves(bond))
ret = __bond_start_xmit(skb, dev);
As slave_cnt could change (and eventually reach 0) between the two
places.
My feeling is that RCU conversion is not properly done in this driver.
Either bond->slave_cnt should be read _once_ for the whole duration of
bond_start_xmit() call, _OR_, be stored in a real Read Copy structure,
so that struct->slave_cnt _cannot_ change during bond_start_xmit()
> res = bond_master_upper_dev_link(bond_dev, slave_dev, new_slave);
> if (res) {
> netdev_dbg(bond_dev, "Error %d calling bond_master_upper_dev_link\n", res);
> @@ -1564,7 +1568,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> goto err_upper_unlink;
> }
>
> - bond->slave_cnt++;
> bond_compute_features(bond);
> bond_set_carrier(bond);
>
> @@ -1590,6 +1593,7 @@ err_upper_unlink:
>
> err_unregister:
> netdev_rx_handler_unregister(slave_dev);
> + bond->slave_cnt--;
>
> err_detach:
> if (!bond_uses_primary(bond))
^ permalink raw reply
* [PATCH v2] net/phy: micrel: Disable asymmetric pause for KSZ9031
From: Mike Looijmans @ 2014-09-12 12:40 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, linux-kernel, Mike Looijmans
In-Reply-To: <5411ECE9.8050600@gmail.com>
The KSZ9031 appears to suffer from the same hardware bug as described
for the KSZ9021 in commit 32fcafbcd1c9f6c7013016a22a5369b4acb93577
("net/phy: micrel: Disable asymmetric pause for KSZ9021")
you have to unplug the cable and plug it back to get it to work.
Remove the SUPPORTED_Asym_Pause flag for the KSZ9031 to fix this.
Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
drivers/net/phy/micrel.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 5a8993b..a932a35 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -425,8 +425,7 @@ static struct phy_driver ksphy_driver[] = {
.phy_id = PHY_ID_KSZ9031,
.phy_id_mask = 0x00fffff0,
.name = "Micrel KSZ9031 Gigabit PHY",
- .features = (PHY_GBIT_FEATURES | SUPPORTED_Pause
- | SUPPORTED_Asym_Pause),
+ .features = (PHY_GBIT_FEATURES | SUPPORTED_Pause),
.flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
.config_init = kszphy_config_init,
.config_aneg = genphy_config_aneg,
--
1.7.9.5
^ permalink raw reply related
* [net-next 10/11] e1000: convert to build_skb
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Florian Westphal, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Florian Westphal <fw@strlen.de>
Instead of preallocating Rx skbs, allocate them right before sending
inbound packet up the stack.
e1000-kvm, mtu1500, netperf TCP_STREAM:
Size Size Size Time Throughput
bytes bytes bytes secs. 10^6bits/sec
old: 87380 16384 16384 60.00 4532.40
new: 87380 16384 16384 60.00 4599.05
Signed-off-by: Florian Westphal <fw@strlen.de>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000.h | 6 +-
drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 29 +--
drivers/net/ethernet/intel/e1000/e1000_main.c | 216 ++++++++++++-----------
3 files changed, 131 insertions(+), 120 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000.h b/drivers/net/ethernet/intel/e1000/e1000.h
index 4c2a102..6970710 100644
--- a/drivers/net/ethernet/intel/e1000/e1000.h
+++ b/drivers/net/ethernet/intel/e1000/e1000.h
@@ -160,9 +160,11 @@ struct e1000_tx_buffer {
};
struct e1000_rx_buffer {
- struct sk_buff *skb;
+ union {
+ struct page *page; /* jumbo: alloc_page */
+ u8 *data; /* else, netdev_alloc_frag */
+ } rxbuf;
dma_addr_t dma;
- struct page *page;
};
struct e1000_tx_ring {
diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
index c7280d9..b691eb4 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
@@ -970,8 +970,7 @@ static void e1000_free_desc_rings(struct e1000_adapter *adapter)
rxdr->buffer_info[i].dma,
E1000_RXBUFFER_2048,
DMA_FROM_DEVICE);
- if (rxdr->buffer_info[i].skb)
- dev_kfree_skb(rxdr->buffer_info[i].skb);
+ kfree(rxdr->buffer_info[i].rxbuf.data);
}
}
@@ -1095,24 +1094,25 @@ static int e1000_setup_desc_rings(struct e1000_adapter *adapter)
for (i = 0; i < rxdr->count; i++) {
struct e1000_rx_desc *rx_desc = E1000_RX_DESC(*rxdr, i);
- struct sk_buff *skb;
+ u8 *buf;
- skb = alloc_skb(E1000_RXBUFFER_2048 + NET_IP_ALIGN, GFP_KERNEL);
- if (!skb) {
+ buf = kzalloc(E1000_RXBUFFER_2048 + NET_SKB_PAD + NET_IP_ALIGN,
+ GFP_KERNEL);
+ if (!buf) {
ret_val = 7;
goto err_nomem;
}
- skb_reserve(skb, NET_IP_ALIGN);
- rxdr->buffer_info[i].skb = skb;
+ rxdr->buffer_info[i].rxbuf.data = buf;
+
rxdr->buffer_info[i].dma =
- dma_map_single(&pdev->dev, skb->data,
+ dma_map_single(&pdev->dev,
+ buf + NET_SKB_PAD + NET_IP_ALIGN,
E1000_RXBUFFER_2048, DMA_FROM_DEVICE);
if (dma_mapping_error(&pdev->dev, rxdr->buffer_info[i].dma)) {
ret_val = 8;
goto err_nomem;
}
rx_desc->buffer_addr = cpu_to_le64(rxdr->buffer_info[i].dma);
- memset(skb->data, 0x00, skb->len);
}
return 0;
@@ -1385,13 +1385,13 @@ static void e1000_create_lbtest_frame(struct sk_buff *skb,
memset(&skb->data[frame_size / 2 + 12], 0xAF, 1);
}
-static int e1000_check_lbtest_frame(struct sk_buff *skb,
+static int e1000_check_lbtest_frame(const unsigned char *data,
unsigned int frame_size)
{
frame_size &= ~1;
- if (skb->data[3] == 0xFF) {
- if (skb->data[frame_size / 2 + 10] == 0xBE &&
- skb->data[frame_size / 2 + 12] == 0xAF) {
+ if (*(data + 3) == 0xFF) {
+ if ((*(data + frame_size / 2 + 10) == 0xBE) &&
+ (*(data + frame_size / 2 + 12) == 0xAF)) {
return 0;
}
}
@@ -1443,7 +1443,8 @@ static int e1000_run_loopback_test(struct e1000_adapter *adapter)
DMA_FROM_DEVICE);
ret_val = e1000_check_lbtest_frame(
- rxdr->buffer_info[l].skb,
+ rxdr->buffer_info[l].rxbuf.data +
+ NET_SKB_PAD + NET_IP_ALIGN,
1024);
if (!ret_val)
good_cnt++;
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index ccb65ad..2ba640a 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -2054,6 +2054,28 @@ void e1000_free_all_rx_resources(struct e1000_adapter *adapter)
e1000_free_rx_resources(adapter, &adapter->rx_ring[i]);
}
+#define E1000_HEADROOM (NET_SKB_PAD + NET_IP_ALIGN)
+static unsigned int e1000_frag_len(const struct e1000_adapter *a)
+{
+ return SKB_DATA_ALIGN(a->rx_buffer_len + E1000_HEADROOM) +
+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+}
+
+static void *e1000_alloc_frag(const struct e1000_adapter *a)
+{
+ unsigned int len = e1000_frag_len(a);
+ u8 *data = netdev_alloc_frag(len);
+
+ if (likely(data))
+ data += E1000_HEADROOM;
+ return data;
+}
+
+static void e1000_free_frag(const void *data)
+{
+ put_page(virt_to_head_page(data));
+}
+
/**
* e1000_clean_rx_ring - Free Rx Buffers per Queue
* @adapter: board private structure
@@ -2068,30 +2090,30 @@ static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
unsigned long size;
unsigned int i;
- /* Free all the Rx ring sk_buffs */
+ /* Free all the Rx netfrags */
for (i = 0; i < rx_ring->count; i++) {
buffer_info = &rx_ring->buffer_info[i];
- if (buffer_info->dma &&
- adapter->clean_rx == e1000_clean_rx_irq) {
- dma_unmap_single(&pdev->dev, buffer_info->dma,
- adapter->rx_buffer_len,
- DMA_FROM_DEVICE);
- } else if (buffer_info->dma &&
- adapter->clean_rx == e1000_clean_jumbo_rx_irq) {
- dma_unmap_page(&pdev->dev, buffer_info->dma,
- adapter->rx_buffer_len,
- DMA_FROM_DEVICE);
+ if (adapter->clean_rx == e1000_clean_rx_irq) {
+ if (buffer_info->dma)
+ dma_unmap_single(&pdev->dev, buffer_info->dma,
+ adapter->rx_buffer_len,
+ DMA_FROM_DEVICE);
+ if (buffer_info->rxbuf.data) {
+ e1000_free_frag(buffer_info->rxbuf.data);
+ buffer_info->rxbuf.data = NULL;
+ }
+ } else if (adapter->clean_rx == e1000_clean_jumbo_rx_irq) {
+ if (buffer_info->dma)
+ dma_unmap_page(&pdev->dev, buffer_info->dma,
+ adapter->rx_buffer_len,
+ DMA_FROM_DEVICE);
+ if (buffer_info->rxbuf.page) {
+ put_page(buffer_info->rxbuf.page);
+ buffer_info->rxbuf.page = NULL;
+ }
}
buffer_info->dma = 0;
- if (buffer_info->page) {
- put_page(buffer_info->page);
- buffer_info->page = NULL;
- }
- if (buffer_info->skb) {
- dev_kfree_skb(buffer_info->skb);
- buffer_info->skb = NULL;
- }
}
/* there also may be some cached data from a chained receive */
@@ -3430,7 +3452,7 @@ rx_ring_summary:
pr_info("R[0x%03X] %016llX %016llX %016llX %p %s\n",
i, le64_to_cpu(u->a), le64_to_cpu(u->b),
- (u64)buffer_info->dma, buffer_info->skb, type);
+ (u64)buffer_info->dma, buffer_info->rxbuf.data, type);
} /* for */
/* dump the descriptor caches */
@@ -3950,12 +3972,12 @@ static void e1000_rx_checksum(struct e1000_adapter *adapter, u32 status_err,
}
/**
- * e1000_consume_page - helper function
+ * e1000_consume_page - helper function for jumbo Rx path
**/
static void e1000_consume_page(struct e1000_rx_buffer *bi, struct sk_buff *skb,
u16 length)
{
- bi->page = NULL;
+ bi->rxbuf.page = NULL;
skb->len += length;
skb->data_len += length;
skb->truesize += PAGE_SIZE;
@@ -4111,6 +4133,7 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
int cleaned_count = 0;
bool cleaned = false;
unsigned int total_rx_bytes=0, total_rx_packets=0;
+ static const unsigned int bufsz = 256 - 16; /* for skb_reserve */
i = rx_ring->next_to_clean;
rx_desc = E1000_RX_DESC(*rx_ring, i);
@@ -4126,8 +4149,6 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
rmb(); /* read descriptor and rx_buffer_info after status DD */
status = rx_desc->status;
- skb = buffer_info->skb;
- buffer_info->skb = NULL;
if (++i == rx_ring->count) i = 0;
next_rxd = E1000_RX_DESC(*rx_ring, i);
@@ -4146,7 +4167,7 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
/* errors is only valid for DD + EOP descriptors */
if (unlikely((status & E1000_RXD_STAT_EOP) &&
(rx_desc->errors & E1000_RXD_ERR_FRAME_ERR_MASK))) {
- u8 *mapped = page_address(buffer_info->page);
+ u8 *mapped = page_address(buffer_info->rxbuf.page);
if (e1000_tbi_should_accept(adapter, status,
rx_desc->errors,
@@ -4155,8 +4176,6 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
} else if (netdev->features & NETIF_F_RXALL) {
goto process_skb;
} else {
- /* recycle both page and skb */
- buffer_info->skb = skb;
/* an error means any chain goes out the window
* too
*/
@@ -4173,16 +4192,18 @@ process_skb:
/* this descriptor is only the beginning (or middle) */
if (!rxtop) {
/* this is the beginning of a chain */
- rxtop = skb;
- skb_fill_page_desc(rxtop, 0, buffer_info->page,
+ rxtop = e1000_alloc_rx_skb(adapter, bufsz);
+ if (!rxtop)
+ break;
+
+ skb_fill_page_desc(rxtop, 0,
+ buffer_info->rxbuf.page,
0, length);
} else {
/* this is the middle of a chain */
skb_fill_page_desc(rxtop,
skb_shinfo(rxtop)->nr_frags,
- buffer_info->page, 0, length);
- /* re-use the skb, only consumed the page */
- buffer_info->skb = skb;
+ buffer_info->rxbuf.page, 0, length);
}
e1000_consume_page(buffer_info, rxtop, length);
goto next_desc;
@@ -4191,32 +4212,33 @@ process_skb:
/* end of the chain */
skb_fill_page_desc(rxtop,
skb_shinfo(rxtop)->nr_frags,
- buffer_info->page, 0, length);
- /* re-use the current skb, we only consumed the
- * page
- */
- buffer_info->skb = skb;
+ buffer_info->rxbuf.page, 0, length);
skb = rxtop;
rxtop = NULL;
e1000_consume_page(buffer_info, skb, length);
} else {
+ struct page *p;
/* no chain, got EOP, this buf is the packet
* copybreak to save the put_page/alloc_page
*/
+ skb = e1000_alloc_rx_skb(adapter, bufsz);
+ if (!skb)
+ break;
+ p = buffer_info->rxbuf.page;
if (length <= copybreak &&
skb_tailroom(skb) >= length) {
u8 *vaddr;
- vaddr = kmap_atomic(buffer_info->page);
+
+ vaddr = kmap_atomic(p);
memcpy(skb_tail_pointer(skb), vaddr,
length);
kunmap_atomic(vaddr);
/* re-use the page, so don't erase
- * buffer_info->page
+ * buffer_info->rxbuf.page
*/
skb_put(skb, length);
} else {
- skb_fill_page_desc(skb, 0,
- buffer_info->page, 0,
+ skb_fill_page_desc(skb, 0, p, 0,
length);
e1000_consume_page(buffer_info, skb,
length);
@@ -4321,6 +4343,7 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
while (rx_desc->status & E1000_RXD_STAT_DD) {
struct sk_buff *skb;
+ u8 *data;
u8 status;
if (*work_done >= work_to_do)
@@ -4331,16 +4354,24 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
status = rx_desc->status;
length = le16_to_cpu(rx_desc->length);
- prefetch(buffer_info->skb->data - NET_IP_ALIGN);
- skb = e1000_copybreak(adapter, buffer_info, length,
- buffer_info->skb->data);
+ data = buffer_info->rxbuf.data;
+ prefetch(data);
+ skb = e1000_copybreak(adapter, buffer_info, length, data);
if (!skb) {
- skb = buffer_info->skb;
- buffer_info->skb = NULL;
+ unsigned int frag_len = e1000_frag_len(adapter);
+
+ skb = build_skb(data - E1000_HEADROOM, frag_len);
+ if (!skb) {
+ adapter->alloc_rx_buff_failed++;
+ break;
+ }
+
+ skb_reserve(skb, E1000_HEADROOM);
dma_unmap_single(&pdev->dev, buffer_info->dma,
adapter->rx_buffer_len,
DMA_FROM_DEVICE);
buffer_info->dma = 0;
+ buffer_info->rxbuf.data = NULL;
}
if (++i == rx_ring->count) i = 0;
@@ -4373,7 +4404,7 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
if (unlikely(rx_desc->errors & E1000_RXD_ERR_FRAME_ERR_MASK)) {
if (e1000_tbi_should_accept(adapter, status,
rx_desc->errors,
- length, skb->data)) {
+ length, data)) {
length--;
} else if (netdev->features & NETIF_F_RXALL) {
goto process_skb;
@@ -4393,7 +4424,7 @@ process_skb:
*/
length -= 4;
- if (buffer_info->skb == NULL)
+ if (buffer_info->rxbuf.data == NULL)
skb_put(skb, length);
else /* copybreak skb */
skb_trim(skb, length);
@@ -4442,37 +4473,19 @@ static void
e1000_alloc_jumbo_rx_buffers(struct e1000_adapter *adapter,
struct e1000_rx_ring *rx_ring, int cleaned_count)
{
- struct net_device *netdev = adapter->netdev;
struct pci_dev *pdev = adapter->pdev;
struct e1000_rx_desc *rx_desc;
struct e1000_rx_buffer *buffer_info;
- struct sk_buff *skb;
unsigned int i;
- unsigned int bufsz = 256 - 16 /*for skb_reserve */ ;
i = rx_ring->next_to_use;
buffer_info = &rx_ring->buffer_info[i];
while (cleaned_count--) {
- skb = buffer_info->skb;
- if (skb) {
- skb_trim(skb, 0);
- goto check_page;
- }
-
- skb = netdev_alloc_skb_ip_align(netdev, bufsz);
- if (unlikely(!skb)) {
- /* Better luck next round */
- adapter->alloc_rx_buff_failed++;
- break;
- }
-
- buffer_info->skb = skb;
-check_page:
/* allocate a new page if necessary */
- if (!buffer_info->page) {
- buffer_info->page = alloc_page(GFP_ATOMIC);
- if (unlikely(!buffer_info->page)) {
+ if (!buffer_info->rxbuf.page) {
+ buffer_info->rxbuf.page = alloc_page(GFP_ATOMIC);
+ if (unlikely(!buffer_info->rxbuf.page)) {
adapter->alloc_rx_buff_failed++;
break;
}
@@ -4480,17 +4493,15 @@ check_page:
if (!buffer_info->dma) {
buffer_info->dma = dma_map_page(&pdev->dev,
- buffer_info->page, 0,
- PAGE_SIZE,
+ buffer_info->rxbuf.page, 0,
+ adapter->rx_buffer_len,
DMA_FROM_DEVICE);
if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
- put_page(buffer_info->page);
- dev_kfree_skb(skb);
- buffer_info->page = NULL;
- buffer_info->skb = NULL;
+ put_page(buffer_info->rxbuf.page);
+ buffer_info->rxbuf.page = NULL;
buffer_info->dma = 0;
adapter->alloc_rx_buff_failed++;
- break; /* while !buffer_info->skb */
+ break;
}
}
@@ -4526,11 +4537,9 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
int cleaned_count)
{
struct e1000_hw *hw = &adapter->hw;
- struct net_device *netdev = adapter->netdev;
struct pci_dev *pdev = adapter->pdev;
struct e1000_rx_desc *rx_desc;
struct e1000_rx_buffer *buffer_info;
- struct sk_buff *skb;
unsigned int i;
unsigned int bufsz = adapter->rx_buffer_len;
@@ -4538,55 +4547,52 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
buffer_info = &rx_ring->buffer_info[i];
while (cleaned_count--) {
- skb = buffer_info->skb;
- if (skb) {
- skb_trim(skb, 0);
+ void *data;
+
+ if (buffer_info->rxbuf.data)
goto skip;
- }
- skb = netdev_alloc_skb_ip_align(netdev, bufsz);
- if (unlikely(!skb)) {
+ data = e1000_alloc_frag(adapter);
+ if (!data) {
/* Better luck next round */
adapter->alloc_rx_buff_failed++;
break;
}
/* Fix for errata 23, can't cross 64kB boundary */
- if (!e1000_check_64k_bound(adapter, skb->data, bufsz)) {
- struct sk_buff *oldskb = skb;
+ if (!e1000_check_64k_bound(adapter, data, bufsz)) {
+ void *olddata = data;
e_err(rx_err, "skb align check failed: %u bytes at "
- "%p\n", bufsz, skb->data);
+ "%p\n", bufsz, data);
/* Try again, without freeing the previous */
- skb = netdev_alloc_skb_ip_align(netdev, bufsz);
+ data = e1000_alloc_frag(adapter);
/* Failed allocation, critical failure */
- if (!skb) {
- dev_kfree_skb(oldskb);
+ if (!data) {
+ e1000_free_frag(olddata);
adapter->alloc_rx_buff_failed++;
break;
}
- if (!e1000_check_64k_bound(adapter, skb->data, bufsz)) {
+ if (!e1000_check_64k_bound(adapter, data, bufsz)) {
/* give up */
- dev_kfree_skb(skb);
- dev_kfree_skb(oldskb);
+ e1000_free_frag(data);
+ e1000_free_frag(olddata);
adapter->alloc_rx_buff_failed++;
- break; /* while !buffer_info->skb */
+ break;
}
/* Use new allocation */
- dev_kfree_skb(oldskb);
+ e1000_free_frag(olddata);
}
- buffer_info->skb = skb;
buffer_info->dma = dma_map_single(&pdev->dev,
- skb->data,
+ data,
adapter->rx_buffer_len,
DMA_FROM_DEVICE);
if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
- dev_kfree_skb(skb);
- buffer_info->skb = NULL;
+ e1000_free_frag(data);
buffer_info->dma = 0;
adapter->alloc_rx_buff_failed++;
- break; /* while !buffer_info->skb */
+ break;
}
/* XXX if it was allocated cleanly it will never map to a
@@ -4600,21 +4606,23 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
e_err(rx_err, "dma align check failed: %u bytes at "
"%p\n", adapter->rx_buffer_len,
(void *)(unsigned long)buffer_info->dma);
- dev_kfree_skb(skb);
- buffer_info->skb = NULL;
dma_unmap_single(&pdev->dev, buffer_info->dma,
adapter->rx_buffer_len,
DMA_FROM_DEVICE);
+
+ e1000_free_frag(data);
+ buffer_info->rxbuf.data = NULL;
buffer_info->dma = 0;
adapter->alloc_rx_buff_failed++;
- break; /* while !buffer_info->skb */
+ break;
}
+ buffer_info->rxbuf.data = data;
+ skip:
rx_desc = E1000_RX_DESC(*rx_ring, i);
rx_desc->buffer_addr = cpu_to_le64(buffer_info->dma);
-skip:
if (unlikely(++i == rx_ring->count))
i = 0;
buffer_info = &rx_ring->buffer_info[i];
--
1.9.3
^ permalink raw reply related
* [net-next 08/11] e1000: add and use e1000_rx_buffer info for Rx
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Florian Westphal, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Florian Westphal <fw@strlen.de>
e1000 uses the same metadata struct for Rx and Tx. But Tx and Rx have
different requirements.
For Rx, we only need to store a buffer and a DMA address.
Follow-up patch will remove skb for Rx, bringing rx_buffer_info down
to 16 bytes on x86_64.
[ buffer_info is 48 bytes ]
Signed-off-by: Florian Westphal <fw@strlen.de>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000.h | 8 +++++-
drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 7 ++---
drivers/net/ethernet/intel/e1000/e1000_main.c | 35 ++++++++++++------------
3 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000.h b/drivers/net/ethernet/intel/e1000/e1000.h
index 10a0f22..81efe33 100644
--- a/drivers/net/ethernet/intel/e1000/e1000.h
+++ b/drivers/net/ethernet/intel/e1000/e1000.h
@@ -160,6 +160,12 @@ struct e1000_buffer {
u16 mapped_as_page;
};
+struct e1000_rx_buffer {
+ struct sk_buff *skb;
+ dma_addr_t dma;
+ struct page *page;
+};
+
struct e1000_tx_ring {
/* pointer to the descriptor ring memory */
void *desc;
@@ -195,7 +201,7 @@ struct e1000_rx_ring {
/* next descriptor to check for DD status bit */
unsigned int next_to_clean;
/* array of buffer information structs */
- struct e1000_buffer *buffer_info;
+ struct e1000_rx_buffer *buffer_info;
struct sk_buff *rx_skb_top;
/* cpu for rx queue */
diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
index 9b50272..5ebc240 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
@@ -968,7 +968,7 @@ static void e1000_free_desc_rings(struct e1000_adapter *adapter)
if (rxdr->buffer_info[i].dma)
dma_unmap_single(&pdev->dev,
rxdr->buffer_info[i].dma,
- rxdr->buffer_info[i].length,
+ E1000_RXBUFFER_2048,
DMA_FROM_DEVICE);
if (rxdr->buffer_info[i].skb)
dev_kfree_skb(rxdr->buffer_info[i].skb);
@@ -1065,7 +1065,7 @@ static int e1000_setup_desc_rings(struct e1000_adapter *adapter)
if (!rxdr->count)
rxdr->count = E1000_DEFAULT_RXD;
- rxdr->buffer_info = kcalloc(rxdr->count, sizeof(struct e1000_buffer),
+ rxdr->buffer_info = kcalloc(rxdr->count, sizeof(struct e1000_rx_buffer),
GFP_KERNEL);
if (!rxdr->buffer_info) {
ret_val = 5;
@@ -1104,7 +1104,6 @@ static int e1000_setup_desc_rings(struct e1000_adapter *adapter)
}
skb_reserve(skb, NET_IP_ALIGN);
rxdr->buffer_info[i].skb = skb;
- rxdr->buffer_info[i].length = E1000_RXBUFFER_2048;
rxdr->buffer_info[i].dma =
dma_map_single(&pdev->dev, skb->data,
E1000_RXBUFFER_2048, DMA_FROM_DEVICE);
@@ -1440,7 +1439,7 @@ static int e1000_run_loopback_test(struct e1000_adapter *adapter)
do { /* receive the sent packets */
dma_sync_single_for_cpu(&pdev->dev,
rxdr->buffer_info[l].dma,
- rxdr->buffer_info[l].length,
+ E1000_RXBUFFER_2048,
DMA_FROM_DEVICE);
ret_val = e1000_check_lbtest_frame(
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 79626ba..694db74 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1687,7 +1687,7 @@ static int e1000_setup_rx_resources(struct e1000_adapter *adapter,
struct pci_dev *pdev = adapter->pdev;
int size, desc_len;
- size = sizeof(struct e1000_buffer) * rxdr->count;
+ size = sizeof(struct e1000_rx_buffer) * rxdr->count;
rxdr->buffer_info = vzalloc(size);
if (!rxdr->buffer_info)
return -ENOMEM;
@@ -2062,7 +2062,7 @@ static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
struct e1000_rx_ring *rx_ring)
{
struct e1000_hw *hw = &adapter->hw;
- struct e1000_buffer *buffer_info;
+ struct e1000_rx_buffer *buffer_info;
struct pci_dev *pdev = adapter->pdev;
unsigned long size;
unsigned int i;
@@ -2073,12 +2073,12 @@ static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
if (buffer_info->dma &&
adapter->clean_rx == e1000_clean_rx_irq) {
dma_unmap_single(&pdev->dev, buffer_info->dma,
- buffer_info->length,
+ adapter->rx_buffer_len,
DMA_FROM_DEVICE);
} else if (buffer_info->dma &&
adapter->clean_rx == e1000_clean_jumbo_rx_irq) {
dma_unmap_page(&pdev->dev, buffer_info->dma,
- buffer_info->length,
+ adapter->rx_buffer_len,
DMA_FROM_DEVICE);
}
@@ -2099,7 +2099,7 @@ static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
rx_ring->rx_skb_top = NULL;
}
- size = sizeof(struct e1000_buffer) * rx_ring->count;
+ size = sizeof(struct e1000_rx_buffer) * rx_ring->count;
memset(rx_ring->buffer_info, 0, size);
/* Zero out the descriptor ring */
@@ -3415,7 +3415,7 @@ rx_ring_summary:
for (i = 0; rx_ring->desc && (i < rx_ring->count); i++) {
struct e1000_rx_desc *rx_desc = E1000_RX_DESC(*rx_ring, i);
- struct e1000_buffer *buffer_info = &rx_ring->buffer_info[i];
+ struct e1000_rx_buffer *buffer_info = &rx_ring->buffer_info[i];
struct my_u { __le64 a; __le64 b; };
struct my_u *u = (struct my_u *)rx_desc;
const char *type;
@@ -3951,7 +3951,7 @@ static void e1000_rx_checksum(struct e1000_adapter *adapter, u32 status_err,
/**
* e1000_consume_page - helper function
**/
-static void e1000_consume_page(struct e1000_buffer *bi, struct sk_buff *skb,
+static void e1000_consume_page(struct e1000_rx_buffer *bi, struct sk_buff *skb,
u16 length)
{
bi->page = NULL;
@@ -4104,7 +4104,7 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
struct net_device *netdev = adapter->netdev;
struct pci_dev *pdev = adapter->pdev;
struct e1000_rx_desc *rx_desc, *next_rxd;
- struct e1000_buffer *buffer_info, *next_buffer;
+ struct e1000_rx_buffer *buffer_info, *next_buffer;
u32 length;
unsigned int i;
int cleaned_count = 0;
@@ -4137,7 +4137,7 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
cleaned = true;
cleaned_count++;
dma_unmap_page(&pdev->dev, buffer_info->dma,
- buffer_info->length, DMA_FROM_DEVICE);
+ adapter->rx_buffer_len, DMA_FROM_DEVICE);
buffer_info->dma = 0;
length = le16_to_cpu(rx_desc->length);
@@ -4273,7 +4273,7 @@ next_desc:
* of reassembly being done in the stack
*/
static struct sk_buff *e1000_copybreak(struct e1000_adapter *adapter,
- struct e1000_buffer *buffer_info,
+ struct e1000_rx_buffer *buffer_info,
u32 length, const void *data)
{
struct sk_buff *skb;
@@ -4307,7 +4307,7 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
struct net_device *netdev = adapter->netdev;
struct pci_dev *pdev = adapter->pdev;
struct e1000_rx_desc *rx_desc, *next_rxd;
- struct e1000_buffer *buffer_info, *next_buffer;
+ struct e1000_rx_buffer *buffer_info, *next_buffer;
u32 length;
unsigned int i;
int cleaned_count = 0;
@@ -4337,7 +4337,8 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
skb = buffer_info->skb;
buffer_info->skb = NULL;
dma_unmap_single(&pdev->dev, buffer_info->dma,
- buffer_info->length, DMA_FROM_DEVICE);
+ adapter->rx_buffer_len,
+ DMA_FROM_DEVICE);
buffer_info->dma = 0;
}
@@ -4443,7 +4444,7 @@ e1000_alloc_jumbo_rx_buffers(struct e1000_adapter *adapter,
struct net_device *netdev = adapter->netdev;
struct pci_dev *pdev = adapter->pdev;
struct e1000_rx_desc *rx_desc;
- struct e1000_buffer *buffer_info;
+ struct e1000_rx_buffer *buffer_info;
struct sk_buff *skb;
unsigned int i;
unsigned int bufsz = 256 - 16 /*for skb_reserve */ ;
@@ -4466,7 +4467,6 @@ e1000_alloc_jumbo_rx_buffers(struct e1000_adapter *adapter,
}
buffer_info->skb = skb;
- buffer_info->length = adapter->rx_buffer_len;
check_page:
/* allocate a new page if necessary */
if (!buffer_info->page) {
@@ -4480,7 +4480,7 @@ check_page:
if (!buffer_info->dma) {
buffer_info->dma = dma_map_page(&pdev->dev,
buffer_info->page, 0,
- buffer_info->length,
+ PAGE_SIZE,
DMA_FROM_DEVICE);
if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
put_page(buffer_info->page);
@@ -4528,7 +4528,7 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
struct net_device *netdev = adapter->netdev;
struct pci_dev *pdev = adapter->pdev;
struct e1000_rx_desc *rx_desc;
- struct e1000_buffer *buffer_info;
+ struct e1000_rx_buffer *buffer_info;
struct sk_buff *skb;
unsigned int i;
unsigned int bufsz = adapter->rx_buffer_len;
@@ -4576,10 +4576,9 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
dev_kfree_skb(oldskb);
}
buffer_info->skb = skb;
- buffer_info->length = adapter->rx_buffer_len;
buffer_info->dma = dma_map_single(&pdev->dev,
skb->data,
- buffer_info->length,
+ adapter->rx_buffer_len,
DMA_FROM_DEVICE);
if (dma_mapping_error(&pdev->dev, buffer_info->dma)) {
dev_kfree_skb(skb);
--
1.9.3
^ permalink raw reply related
* [net-next 11/11] e1000: switch to napi_gro_frags api
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Florian Westphal, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Florian Westphal <fw@strlen.de>
napi_gro_frags allows skb re-use in case GRO can merge payload pages
into an skb on the GRO lists.
netperf TCP_STREAM, kvm-e1000 emulation, mtu 9k:
Size Size Size Time Throughput
bytes bytes bytes secs. 10^6bits/sec
old: 87380 16384 16384 30.00 8985.78
new: 87380 16384 16384 30.00 9907.05
Signed-off-by: Florian Westphal <fw@strlen.de>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 49 +++++++++++++++++----------
1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 2ba640a..5f6aded 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -2117,10 +2117,8 @@ static void e1000_clean_rx_ring(struct e1000_adapter *adapter,
}
/* there also may be some cached data from a chained receive */
- if (rx_ring->rx_skb_top) {
- dev_kfree_skb(rx_ring->rx_skb_top);
- rx_ring->rx_skb_top = NULL;
- }
+ napi_free_frags(&adapter->napi);
+ rx_ring->rx_skb_top = NULL;
size = sizeof(struct e1000_rx_buffer) * rx_ring->count;
memset(rx_ring->buffer_info, 0, size);
@@ -4133,7 +4131,6 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
int cleaned_count = 0;
bool cleaned = false;
unsigned int total_rx_bytes=0, total_rx_packets=0;
- static const unsigned int bufsz = 256 - 16; /* for skb_reserve */
i = rx_ring->next_to_clean;
rx_desc = E1000_RX_DESC(*rx_ring, i);
@@ -4192,7 +4189,7 @@ process_skb:
/* this descriptor is only the beginning (or middle) */
if (!rxtop) {
/* this is the beginning of a chain */
- rxtop = e1000_alloc_rx_skb(adapter, bufsz);
+ rxtop = napi_get_frags(&adapter->napi);
if (!rxtop)
break;
@@ -4221,14 +4218,17 @@ process_skb:
/* no chain, got EOP, this buf is the packet
* copybreak to save the put_page/alloc_page
*/
- skb = e1000_alloc_rx_skb(adapter, bufsz);
- if (!skb)
- break;
p = buffer_info->rxbuf.page;
- if (length <= copybreak &&
- skb_tailroom(skb) >= length) {
+ if (length <= copybreak) {
u8 *vaddr;
+ if (likely(!(netdev->features & NETIF_F_RXFCS)))
+ length -= 4;
+ skb = e1000_alloc_rx_skb(adapter,
+ length);
+ if (!skb)
+ break;
+
vaddr = kmap_atomic(p);
memcpy(skb_tail_pointer(skb), vaddr,
length);
@@ -4237,7 +4237,22 @@ process_skb:
* buffer_info->rxbuf.page
*/
skb_put(skb, length);
+ e1000_rx_checksum(adapter,
+ status | rx_desc->errors << 24,
+ le16_to_cpu(rx_desc->csum), skb);
+
+ total_rx_bytes += skb->len;
+ total_rx_packets++;
+
+ e1000_receive_skb(adapter, status,
+ rx_desc->special, skb);
+ goto next_desc;
} else {
+ skb = napi_get_frags(&adapter->napi);
+ if (!skb) {
+ adapter->alloc_rx_buff_failed++;
+ break;
+ }
skb_fill_page_desc(skb, 0, p, 0,
length);
e1000_consume_page(buffer_info, skb,
@@ -4257,14 +4272,14 @@ process_skb:
pskb_trim(skb, skb->len - 4);
total_rx_packets++;
- /* eth type trans needs skb->data to point to something */
- if (!pskb_may_pull(skb, ETH_HLEN)) {
- e_err(drv, "pskb_may_pull failed.\n");
- dev_kfree_skb(skb);
- goto next_desc;
+ if (status & E1000_RXD_STAT_VP) {
+ __le16 vlan = rx_desc->special;
+ u16 vid = le16_to_cpu(vlan) & E1000_RXD_SPC_VLAN_MASK;
+
+ __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
}
- e1000_receive_skb(adapter, status, rx_desc->special, skb);
+ napi_gro_frags(&adapter->napi);
next_desc:
rx_desc->status = 0;
--
1.9.3
^ permalink raw reply related
* [net-next 09/11] e1000: rename struct e1000_buffer to e1000_tx_buffer
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Florian Westphal, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Florian Westphal <fw@strlen.de>
and remove *page, its only used for Rx.
Signed-off-by: Florian Westphal <fw@strlen.de>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000.h | 9 ++++-----
drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
drivers/net/ethernet/intel/e1000/e1000_main.c | 23 ++++++++++++-----------
3 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000.h b/drivers/net/ethernet/intel/e1000/e1000.h
index 81efe33..4c2a102 100644
--- a/drivers/net/ethernet/intel/e1000/e1000.h
+++ b/drivers/net/ethernet/intel/e1000/e1000.h
@@ -148,16 +148,15 @@ struct e1000_adapter;
/* wrapper around a pointer to a socket buffer,
* so a DMA handle can be stored along with the buffer
*/
-struct e1000_buffer {
+struct e1000_tx_buffer {
struct sk_buff *skb;
dma_addr_t dma;
- struct page *page;
unsigned long time_stamp;
u16 length;
u16 next_to_watch;
- unsigned int segs;
+ bool mapped_as_page;
+ unsigned short segs;
unsigned int bytecount;
- u16 mapped_as_page;
};
struct e1000_rx_buffer {
@@ -180,7 +179,7 @@ struct e1000_tx_ring {
/* next descriptor to check for DD status bit */
unsigned int next_to_clean;
/* array of buffer information structs */
- struct e1000_buffer *buffer_info;
+ struct e1000_tx_buffer *buffer_info;
u16 tdh;
u16 tdt;
diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
index 5ebc240..c7280d9 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
@@ -1006,7 +1006,7 @@ static int e1000_setup_desc_rings(struct e1000_adapter *adapter)
if (!txdr->count)
txdr->count = E1000_DEFAULT_TXD;
- txdr->buffer_info = kcalloc(txdr->count, sizeof(struct e1000_buffer),
+ txdr->buffer_info = kcalloc(txdr->count, sizeof(struct e1000_tx_buffer),
GFP_KERNEL);
if (!txdr->buffer_info) {
ret_val = 1;
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 694db74..ccb65ad 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1497,7 +1497,7 @@ static int e1000_setup_tx_resources(struct e1000_adapter *adapter,
struct pci_dev *pdev = adapter->pdev;
int size;
- size = sizeof(struct e1000_buffer) * txdr->count;
+ size = sizeof(struct e1000_tx_buffer) * txdr->count;
txdr->buffer_info = vzalloc(size);
if (!txdr->buffer_info)
return -ENOMEM;
@@ -1947,8 +1947,9 @@ void e1000_free_all_tx_resources(struct e1000_adapter *adapter)
e1000_free_tx_resources(adapter, &adapter->tx_ring[i]);
}
-static void e1000_unmap_and_free_tx_resource(struct e1000_adapter *adapter,
- struct e1000_buffer *buffer_info)
+static void
+e1000_unmap_and_free_tx_resource(struct e1000_adapter *adapter,
+ struct e1000_tx_buffer *buffer_info)
{
if (buffer_info->dma) {
if (buffer_info->mapped_as_page)
@@ -1977,7 +1978,7 @@ static void e1000_clean_tx_ring(struct e1000_adapter *adapter,
struct e1000_tx_ring *tx_ring)
{
struct e1000_hw *hw = &adapter->hw;
- struct e1000_buffer *buffer_info;
+ struct e1000_tx_buffer *buffer_info;
unsigned long size;
unsigned int i;
@@ -1989,7 +1990,7 @@ static void e1000_clean_tx_ring(struct e1000_adapter *adapter,
}
netdev_reset_queue(adapter->netdev);
- size = sizeof(struct e1000_buffer) * tx_ring->count;
+ size = sizeof(struct e1000_tx_buffer) * tx_ring->count;
memset(tx_ring->buffer_info, 0, size);
/* Zero out the descriptor ring */
@@ -2678,7 +2679,7 @@ static int e1000_tso(struct e1000_adapter *adapter,
__be16 protocol)
{
struct e1000_context_desc *context_desc;
- struct e1000_buffer *buffer_info;
+ struct e1000_tx_buffer *buffer_info;
unsigned int i;
u32 cmd_length = 0;
u16 ipcse = 0, tucse, mss;
@@ -2750,7 +2751,7 @@ static bool e1000_tx_csum(struct e1000_adapter *adapter,
__be16 protocol)
{
struct e1000_context_desc *context_desc;
- struct e1000_buffer *buffer_info;
+ struct e1000_tx_buffer *buffer_info;
unsigned int i;
u8 css;
u32 cmd_len = E1000_TXD_CMD_DEXT;
@@ -2809,7 +2810,7 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
{
struct e1000_hw *hw = &adapter->hw;
struct pci_dev *pdev = adapter->pdev;
- struct e1000_buffer *buffer_info;
+ struct e1000_tx_buffer *buffer_info;
unsigned int len = skb_headlen(skb);
unsigned int offset = 0, size, count = 0, i;
unsigned int f, bytecount, segs;
@@ -2955,7 +2956,7 @@ static void e1000_tx_queue(struct e1000_adapter *adapter,
{
struct e1000_hw *hw = &adapter->hw;
struct e1000_tx_desc *tx_desc = NULL;
- struct e1000_buffer *buffer_info;
+ struct e1000_tx_buffer *buffer_info;
u32 txd_upper = 0, txd_lower = E1000_TXD_CMD_IFCS;
unsigned int i;
@@ -3373,7 +3374,7 @@ static void e1000_dump(struct e1000_adapter *adapter)
for (i = 0; tx_ring->desc && (i < tx_ring->count); i++) {
struct e1000_tx_desc *tx_desc = E1000_TX_DESC(*tx_ring, i);
- struct e1000_buffer *buffer_info = &tx_ring->buffer_info[i];
+ struct e1000_tx_buffer *buffer_info = &tx_ring->buffer_info[i];
struct my_u { __le64 a; __le64 b; };
struct my_u *u = (struct my_u *)tx_desc;
const char *type;
@@ -3811,7 +3812,7 @@ static bool e1000_clean_tx_irq(struct e1000_adapter *adapter,
struct e1000_hw *hw = &adapter->hw;
struct net_device *netdev = adapter->netdev;
struct e1000_tx_desc *tx_desc, *eop_desc;
- struct e1000_buffer *buffer_info;
+ struct e1000_tx_buffer *buffer_info;
unsigned int i, eop;
unsigned int count = 0;
unsigned int total_tx_bytes=0, total_tx_packets=0;
--
1.9.3
^ permalink raw reply related
* [net-next 07/11] e1000: perform copybreak ahead of DMA unmap
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Florian Westphal, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Florian Westphal <fw@strlen.de>
Currently we unmap the DMA range, then copy to new skb.
Change this so we can keep the mapping in case the data is copied.
Signed-off-by: Florian Westphal <fw@strlen.de>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 73 ++++++++++++++++-----------
1 file changed, 43 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index e26a32d..79626ba 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -4077,6 +4077,16 @@ static bool e1000_tbi_should_accept(struct e1000_adapter *adapter,
return false;
}
+static struct sk_buff *e1000_alloc_rx_skb(struct e1000_adapter *adapter,
+ unsigned int bufsz)
+{
+ struct sk_buff *skb = netdev_alloc_skb_ip_align(adapter->netdev, bufsz);
+
+ if (unlikely(!skb))
+ adapter->alloc_rx_buff_failed++;
+ return skb;
+}
+
/**
* e1000_clean_jumbo_rx_irq - Send received data up the network stack; legacy
* @adapter: board private structure
@@ -4262,25 +4272,25 @@ next_desc:
/* this should improve performance for small packets with large amounts
* of reassembly being done in the stack
*/
-static void e1000_check_copybreak(struct net_device *netdev,
- struct e1000_buffer *buffer_info,
- u32 length, struct sk_buff **skb)
+static struct sk_buff *e1000_copybreak(struct e1000_adapter *adapter,
+ struct e1000_buffer *buffer_info,
+ u32 length, const void *data)
{
- struct sk_buff *new_skb;
+ struct sk_buff *skb;
if (length > copybreak)
- return;
+ return NULL;
- new_skb = netdev_alloc_skb_ip_align(netdev, length);
- if (!new_skb)
- return;
+ skb = e1000_alloc_rx_skb(adapter, length);
+ if (!skb)
+ return NULL;
+
+ dma_sync_single_for_cpu(&adapter->pdev->dev, buffer_info->dma,
+ length, DMA_FROM_DEVICE);
+
+ memcpy(skb_put(skb, length), data, length);
- skb_copy_to_linear_data_offset(new_skb, -NET_IP_ALIGN,
- (*skb)->data - NET_IP_ALIGN,
- length + NET_IP_ALIGN);
- /* save the skb in buffer_info as good */
- buffer_info->skb = *skb;
- *skb = new_skb;
+ return skb;
}
/**
@@ -4318,10 +4328,18 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
rmb(); /* read descriptor and rx_buffer_info after status DD */
status = rx_desc->status;
- skb = buffer_info->skb;
- buffer_info->skb = NULL;
+ length = le16_to_cpu(rx_desc->length);
- prefetch(skb->data - NET_IP_ALIGN);
+ prefetch(buffer_info->skb->data - NET_IP_ALIGN);
+ skb = e1000_copybreak(adapter, buffer_info, length,
+ buffer_info->skb->data);
+ if (!skb) {
+ skb = buffer_info->skb;
+ buffer_info->skb = NULL;
+ dma_unmap_single(&pdev->dev, buffer_info->dma,
+ buffer_info->length, DMA_FROM_DEVICE);
+ buffer_info->dma = 0;
+ }
if (++i == rx_ring->count) i = 0;
next_rxd = E1000_RX_DESC(*rx_ring, i);
@@ -4331,11 +4349,7 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
cleaned = true;
cleaned_count++;
- dma_unmap_single(&pdev->dev, buffer_info->dma,
- buffer_info->length, DMA_FROM_DEVICE);
- buffer_info->dma = 0;
- length = le16_to_cpu(rx_desc->length);
/* !EOP means multiple descriptors were used to store a single
* packet, if thats the case we need to toss it. In fact, we
* to toss every packet with the EOP bit clear and the next
@@ -4348,8 +4362,7 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
if (adapter->discarding) {
/* All receives must fit into a single buffer */
netdev_dbg(netdev, "Receive packet consumed multiple buffers\n");
- /* recycle */
- buffer_info->skb = skb;
+ dev_kfree_skb(skb);
if (status & E1000_RXD_STAT_EOP)
adapter->discarding = false;
goto next_desc;
@@ -4363,8 +4376,7 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
} else if (netdev->features & NETIF_F_RXALL) {
goto process_skb;
} else {
- /* recycle */
- buffer_info->skb = skb;
+ dev_kfree_skb(skb);
goto next_desc;
}
}
@@ -4379,9 +4391,10 @@ process_skb:
*/
length -= 4;
- e1000_check_copybreak(netdev, buffer_info, length, &skb);
-
- skb_put(skb, length);
+ if (buffer_info->skb == NULL)
+ skb_put(skb, length);
+ else /* copybreak skb */
+ skb_trim(skb, length);
/* Receive Checksum Offload */
e1000_rx_checksum(adapter,
@@ -4527,7 +4540,7 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
skb = buffer_info->skb;
if (skb) {
skb_trim(skb, 0);
- goto map_skb;
+ goto skip;
}
skb = netdev_alloc_skb_ip_align(netdev, bufsz);
@@ -4564,7 +4577,6 @@ static void e1000_alloc_rx_buffers(struct e1000_adapter *adapter,
}
buffer_info->skb = skb;
buffer_info->length = adapter->rx_buffer_len;
-map_skb:
buffer_info->dma = dma_map_single(&pdev->dev,
skb->data,
buffer_info->length,
@@ -4602,6 +4614,7 @@ map_skb:
rx_desc = E1000_RX_DESC(*rx_ring, i);
rx_desc->buffer_addr = cpu_to_le64(buffer_info->dma);
+skip:
if (unlikely(++i == rx_ring->count))
i = 0;
buffer_info = &rx_ring->buffer_info[i];
--
1.9.3
^ permalink raw reply related
* [net-next 05/11] e1000: move e1000_tbi_adjust_stats to where its used
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Florian Westphal, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Florian Westphal <fw@strlen.de>
... and make it static.
Signed-off-by: Florian Westphal <fw@strlen.de>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000_hw.c | 78 ---------------------------
drivers/net/ethernet/intel/e1000/e1000_hw.h | 2 -
drivers/net/ethernet/intel/e1000/e1000_main.c | 77 ++++++++++++++++++++++++++
3 files changed, 77 insertions(+), 80 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
index 1acf503..45c8c864 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
@@ -4837,84 +4837,6 @@ void e1000_update_adaptive(struct e1000_hw *hw)
}
/**
- * e1000_tbi_adjust_stats
- * @hw: Struct containing variables accessed by shared code
- * @frame_len: The length of the frame in question
- * @mac_addr: The Ethernet destination address of the frame in question
- *
- * Adjusts the statistic counters when a frame is accepted by TBI_ACCEPT
- */
-void e1000_tbi_adjust_stats(struct e1000_hw *hw, struct e1000_hw_stats *stats,
- u32 frame_len, u8 *mac_addr)
-{
- u64 carry_bit;
-
- /* First adjust the frame length. */
- frame_len--;
- /* We need to adjust the statistics counters, since the hardware
- * counters overcount this packet as a CRC error and undercount
- * the packet as a good packet
- */
- /* This packet should not be counted as a CRC error. */
- stats->crcerrs--;
- /* This packet does count as a Good Packet Received. */
- stats->gprc++;
-
- /* Adjust the Good Octets received counters */
- carry_bit = 0x80000000 & stats->gorcl;
- stats->gorcl += frame_len;
- /* If the high bit of Gorcl (the low 32 bits of the Good Octets
- * Received Count) was one before the addition,
- * AND it is zero after, then we lost the carry out,
- * need to add one to Gorch (Good Octets Received Count High).
- * This could be simplified if all environments supported
- * 64-bit integers.
- */
- if (carry_bit && ((stats->gorcl & 0x80000000) == 0))
- stats->gorch++;
- /* Is this a broadcast or multicast? Check broadcast first,
- * since the test for a multicast frame will test positive on
- * a broadcast frame.
- */
- if (is_broadcast_ether_addr(mac_addr))
- /* Broadcast packet */
- stats->bprc++;
- else if (is_multicast_ether_addr(mac_addr))
- /* Multicast packet */
- stats->mprc++;
-
- if (frame_len == hw->max_frame_size) {
- /* In this case, the hardware has overcounted the number of
- * oversize frames.
- */
- if (stats->roc > 0)
- stats->roc--;
- }
-
- /* Adjust the bin counters when the extra byte put the frame in the
- * wrong bin. Remember that the frame_len was adjusted above.
- */
- if (frame_len == 64) {
- stats->prc64++;
- stats->prc127--;
- } else if (frame_len == 127) {
- stats->prc127++;
- stats->prc255--;
- } else if (frame_len == 255) {
- stats->prc255++;
- stats->prc511--;
- } else if (frame_len == 511) {
- stats->prc511++;
- stats->prc1023--;
- } else if (frame_len == 1023) {
- stats->prc1023++;
- stats->prc1522--;
- } else if (frame_len == 1522) {
- stats->prc1522++;
- }
-}
-
-/**
* e1000_get_bus_info
* @hw: Struct containing variables accessed by shared code
*
diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.h b/drivers/net/ethernet/intel/e1000/e1000_hw.h
index 11578c8..5cf7268 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.h
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.h
@@ -393,8 +393,6 @@ s32 e1000_blink_led_start(struct e1000_hw *hw);
/* Everything else */
void e1000_reset_adaptive(struct e1000_hw *hw);
void e1000_update_adaptive(struct e1000_hw *hw);
-void e1000_tbi_adjust_stats(struct e1000_hw *hw, struct e1000_hw_stats *stats,
- u32 frame_len, u8 * mac_addr);
void e1000_get_bus_info(struct e1000_hw *hw);
void e1000_pci_set_mwi(struct e1000_hw *hw);
void e1000_pci_clear_mwi(struct e1000_hw *hw);
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index ad3d5d1..fe2b9ad 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -3981,6 +3981,83 @@ static void e1000_receive_skb(struct e1000_adapter *adapter, u8 status,
}
/**
+ * e1000_tbi_adjust_stats
+ * @hw: Struct containing variables accessed by shared code
+ * @frame_len: The length of the frame in question
+ * @mac_addr: The Ethernet destination address of the frame in question
+ *
+ * Adjusts the statistic counters when a frame is accepted by TBI_ACCEPT
+ */
+static void e1000_tbi_adjust_stats(struct e1000_hw *hw,
+ struct e1000_hw_stats *stats,
+ u32 frame_len, const u8 *mac_addr)
+{
+ u64 carry_bit;
+
+ /* First adjust the frame length. */
+ frame_len--;
+ /* We need to adjust the statistics counters, since the hardware
+ * counters overcount this packet as a CRC error and undercount
+ * the packet as a good packet
+ */
+ /* This packet should not be counted as a CRC error. */
+ stats->crcerrs--;
+ /* This packet does count as a Good Packet Received. */
+ stats->gprc++;
+
+ /* Adjust the Good Octets received counters */
+ carry_bit = 0x80000000 & stats->gorcl;
+ stats->gorcl += frame_len;
+ /* If the high bit of Gorcl (the low 32 bits of the Good Octets
+ * Received Count) was one before the addition,
+ * AND it is zero after, then we lost the carry out,
+ * need to add one to Gorch (Good Octets Received Count High).
+ * This could be simplified if all environments supported
+ * 64-bit integers.
+ */
+ if (carry_bit && ((stats->gorcl & 0x80000000) == 0))
+ stats->gorch++;
+ /* Is this a broadcast or multicast? Check broadcast first,
+ * since the test for a multicast frame will test positive on
+ * a broadcast frame.
+ */
+ if (is_broadcast_ether_addr(mac_addr))
+ stats->bprc++;
+ else if (is_multicast_ether_addr(mac_addr))
+ stats->mprc++;
+
+ if (frame_len == hw->max_frame_size) {
+ /* In this case, the hardware has overcounted the number of
+ * oversize frames.
+ */
+ if (stats->roc > 0)
+ stats->roc--;
+ }
+
+ /* Adjust the bin counters when the extra byte put the frame in the
+ * wrong bin. Remember that the frame_len was adjusted above.
+ */
+ if (frame_len == 64) {
+ stats->prc64++;
+ stats->prc127--;
+ } else if (frame_len == 127) {
+ stats->prc127++;
+ stats->prc255--;
+ } else if (frame_len == 255) {
+ stats->prc255++;
+ stats->prc511--;
+ } else if (frame_len == 511) {
+ stats->prc511++;
+ stats->prc1023--;
+ } else if (frame_len == 1023) {
+ stats->prc1023++;
+ stats->prc1522--;
+ } else if (frame_len == 1522) {
+ stats->prc1522++;
+ }
+}
+
+/**
* e1000_clean_jumbo_rx_irq - Send received data up the network stack; legacy
* @adapter: board private structure
* @rx_ring: ring to clean
--
1.9.3
^ permalink raw reply related
* [net-next 06/11] e1000: move tbi workaround code into helper function
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Florian Westphal, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Florian Westphal <fw@strlen.de>
Its the same in both handlers.
Signed-off-by: Florian Westphal <fw@strlen.de>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 63 ++++++++++++++-------------
1 file changed, 33 insertions(+), 30 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index fe2b9ad..e26a32d 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -4057,6 +4057,26 @@ static void e1000_tbi_adjust_stats(struct e1000_hw *hw,
}
}
+static bool e1000_tbi_should_accept(struct e1000_adapter *adapter,
+ u8 status, u8 errors,
+ u32 length, const u8 *data)
+{
+ struct e1000_hw *hw = &adapter->hw;
+ u8 last_byte = *(data + length - 1);
+
+ if (TBI_ACCEPT(hw, status, errors, length, last_byte)) {
+ unsigned long irq_flags;
+
+ spin_lock_irqsave(&adapter->stats_lock, irq_flags);
+ e1000_tbi_adjust_stats(hw, &adapter->stats, length, data);
+ spin_unlock_irqrestore(&adapter->stats_lock, irq_flags);
+
+ return true;
+ }
+
+ return false;
+}
+
/**
* e1000_clean_jumbo_rx_irq - Send received data up the network stack; legacy
* @adapter: board private structure
@@ -4071,12 +4091,10 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
struct e1000_rx_ring *rx_ring,
int *work_done, int work_to_do)
{
- struct e1000_hw *hw = &adapter->hw;
struct net_device *netdev = adapter->netdev;
struct pci_dev *pdev = adapter->pdev;
struct e1000_rx_desc *rx_desc, *next_rxd;
struct e1000_buffer *buffer_info, *next_buffer;
- unsigned long irq_flags;
u32 length;
unsigned int i;
int cleaned_count = 0;
@@ -4117,23 +4135,15 @@ static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
/* errors is only valid for DD + EOP descriptors */
if (unlikely((status & E1000_RXD_STAT_EOP) &&
(rx_desc->errors & E1000_RXD_ERR_FRAME_ERR_MASK))) {
- u8 *mapped;
- u8 last_byte;
-
- mapped = page_address(buffer_info->page);
- last_byte = *(mapped + length - 1);
- if (TBI_ACCEPT(hw, status, rx_desc->errors, length,
- last_byte)) {
- spin_lock_irqsave(&adapter->stats_lock,
- irq_flags);
- e1000_tbi_adjust_stats(hw, &adapter->stats,
- length, mapped);
- spin_unlock_irqrestore(&adapter->stats_lock,
- irq_flags);
+ u8 *mapped = page_address(buffer_info->page);
+
+ if (e1000_tbi_should_accept(adapter, status,
+ rx_desc->errors,
+ length, mapped)) {
length--;
+ } else if (netdev->features & NETIF_F_RXALL) {
+ goto process_skb;
} else {
- if (netdev->features & NETIF_F_RXALL)
- goto process_skb;
/* recycle both page and skb */
buffer_info->skb = skb;
/* an error means any chain goes out the window
@@ -4284,12 +4294,10 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
struct e1000_rx_ring *rx_ring,
int *work_done, int work_to_do)
{
- struct e1000_hw *hw = &adapter->hw;
struct net_device *netdev = adapter->netdev;
struct pci_dev *pdev = adapter->pdev;
struct e1000_rx_desc *rx_desc, *next_rxd;
struct e1000_buffer *buffer_info, *next_buffer;
- unsigned long flags;
u32 length;
unsigned int i;
int cleaned_count = 0;
@@ -4339,7 +4347,7 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
if (adapter->discarding) {
/* All receives must fit into a single buffer */
- e_dbg("Receive packet consumed multiple buffers\n");
+ netdev_dbg(netdev, "Receive packet consumed multiple buffers\n");
/* recycle */
buffer_info->skb = skb;
if (status & E1000_RXD_STAT_EOP)
@@ -4348,18 +4356,13 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
}
if (unlikely(rx_desc->errors & E1000_RXD_ERR_FRAME_ERR_MASK)) {
- u8 last_byte = *(skb->data + length - 1);
- if (TBI_ACCEPT(hw, status, rx_desc->errors, length,
- last_byte)) {
- spin_lock_irqsave(&adapter->stats_lock, flags);
- e1000_tbi_adjust_stats(hw, &adapter->stats,
- length, skb->data);
- spin_unlock_irqrestore(&adapter->stats_lock,
- flags);
+ if (e1000_tbi_should_accept(adapter, status,
+ rx_desc->errors,
+ length, skb->data)) {
length--;
+ } else if (netdev->features & NETIF_F_RXALL) {
+ goto process_skb;
} else {
- if (netdev->features & NETIF_F_RXALL)
- goto process_skb;
/* recycle */
buffer_info->skb = skb;
goto next_desc;
--
1.9.3
^ permalink raw reply related
* [net-next 01/11] ixgbe: Resolve warnings produced in W=2 builds
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Mark Rustad, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Mark Rustad <mark.d.rustad@intel.com>
This patch resolves warnings produced by ixgbe in W=2 kernel
builds. There are missing-field-initializers warnings and shadow
warnings. None of these point to any deeper problem, so just
resolve them so any new warnings get analyzed.
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 4 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 18 +++++++++---------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index e4100b5..cff383b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1303,7 +1303,7 @@ static const struct ixgbe_reg_test reg_test_82599[] = {
{ IXGBE_RAL(0), 16, TABLE64_TEST_LO, 0xFFFFFFFF, 0xFFFFFFFF },
{ IXGBE_RAL(0), 16, TABLE64_TEST_HI, 0x8001FFFF, 0x800CFFFF },
{ IXGBE_MTA(0), 128, TABLE32_TEST, 0xFFFFFFFF, 0xFFFFFFFF },
- { 0, 0, 0, 0 }
+ { .reg = 0 }
};
/* default 82598 register test */
@@ -1331,7 +1331,7 @@ static const struct ixgbe_reg_test reg_test_82598[] = {
{ IXGBE_RAL(0), 16, TABLE64_TEST_LO, 0xFFFFFFFF, 0xFFFFFFFF },
{ IXGBE_RAL(0), 16, TABLE64_TEST_HI, 0x800CFFFF, 0x800CFFFF },
{ IXGBE_MTA(0), 128, TABLE32_TEST, 0xFFFFFFFF, 0xFFFFFFFF },
- { 0, 0, 0, 0 }
+ { .reg = 0 }
};
static bool reg_pattern_test(struct ixgbe_adapter *adapter, u64 *data, int reg,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 166dc00..87613806 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -440,7 +440,7 @@ static const struct ixgbe_reg_info ixgbe_reg_info_tbl[] = {
{IXGBE_TXDCTL(0), "TXDCTL"},
/* List Terminator */
- {}
+ { .name = NULL }
};
@@ -5186,15 +5186,15 @@ int ixgbe_setup_tx_resources(struct ixgbe_ring *tx_ring)
{
struct device *dev = tx_ring->dev;
int orig_node = dev_to_node(dev);
- int numa_node = -1;
+ int ring_node = -1;
int size;
size = sizeof(struct ixgbe_tx_buffer) * tx_ring->count;
if (tx_ring->q_vector)
- numa_node = tx_ring->q_vector->numa_node;
+ ring_node = tx_ring->q_vector->numa_node;
- tx_ring->tx_buffer_info = vzalloc_node(size, numa_node);
+ tx_ring->tx_buffer_info = vzalloc_node(size, ring_node);
if (!tx_ring->tx_buffer_info)
tx_ring->tx_buffer_info = vzalloc(size);
if (!tx_ring->tx_buffer_info)
@@ -5206,7 +5206,7 @@ int ixgbe_setup_tx_resources(struct ixgbe_ring *tx_ring)
tx_ring->size = tx_ring->count * sizeof(union ixgbe_adv_tx_desc);
tx_ring->size = ALIGN(tx_ring->size, 4096);
- set_dev_node(dev, numa_node);
+ set_dev_node(dev, ring_node);
tx_ring->desc = dma_alloc_coherent(dev,
tx_ring->size,
&tx_ring->dma,
@@ -5270,15 +5270,15 @@ int ixgbe_setup_rx_resources(struct ixgbe_ring *rx_ring)
{
struct device *dev = rx_ring->dev;
int orig_node = dev_to_node(dev);
- int numa_node = -1;
+ int ring_node = -1;
int size;
size = sizeof(struct ixgbe_rx_buffer) * rx_ring->count;
if (rx_ring->q_vector)
- numa_node = rx_ring->q_vector->numa_node;
+ ring_node = rx_ring->q_vector->numa_node;
- rx_ring->rx_buffer_info = vzalloc_node(size, numa_node);
+ rx_ring->rx_buffer_info = vzalloc_node(size, ring_node);
if (!rx_ring->rx_buffer_info)
rx_ring->rx_buffer_info = vzalloc(size);
if (!rx_ring->rx_buffer_info)
@@ -5290,7 +5290,7 @@ int ixgbe_setup_rx_resources(struct ixgbe_ring *rx_ring)
rx_ring->size = rx_ring->count * sizeof(union ixgbe_adv_rx_desc);
rx_ring->size = ALIGN(rx_ring->size, 4096);
- set_dev_node(dev, numa_node);
+ set_dev_node(dev, ring_node);
rx_ring->desc = dma_alloc_coherent(dev,
rx_ring->size,
&rx_ring->dma,
--
1.9.3
^ permalink raw reply related
* [net-next 04/11] ixgbe: Refactor busy poll socket code to address multiple issues
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Alexander Duyck, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Alexander Duyck <alexander.h.duyck@intel.com>
This change addresses several issues in the current ixgbe implementation of
busy poll sockets.
First was the fact that it was possible for frames to be delivered out of
order if they were held in GRO. This is addressed by flushing the GRO buffers
before releasing the q_vector back to the idle state.
The other issue was the fact that we were having to take a spinlock on
changing the state to and from idle. To resolve this I have replaced the
state value with an atomic and use atomic_cmpxchg to change the value from
idle, and a simple atomic set to restore it back to idle after we have
acquired it. This allows us to only use a locked operation on acquiring the
vector without a need for a locked operation to release it.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 112 ++++++++++-----------------
drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 5 ++
2 files changed, 45 insertions(+), 72 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index ac9f214..06744f8 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -386,119 +386,87 @@ struct ixgbe_q_vector {
char name[IFNAMSIZ + 9];
#ifdef CONFIG_NET_RX_BUSY_POLL
- unsigned int state;
-#define IXGBE_QV_STATE_IDLE 0
-#define IXGBE_QV_STATE_NAPI 1 /* NAPI owns this QV */
-#define IXGBE_QV_STATE_POLL 2 /* poll owns this QV */
-#define IXGBE_QV_STATE_DISABLED 4 /* QV is disabled */
-#define IXGBE_QV_OWNED (IXGBE_QV_STATE_NAPI | IXGBE_QV_STATE_POLL)
-#define IXGBE_QV_LOCKED (IXGBE_QV_OWNED | IXGBE_QV_STATE_DISABLED)
-#define IXGBE_QV_STATE_NAPI_YIELD 8 /* NAPI yielded this QV */
-#define IXGBE_QV_STATE_POLL_YIELD 16 /* poll yielded this QV */
-#define IXGBE_QV_YIELD (IXGBE_QV_STATE_NAPI_YIELD | IXGBE_QV_STATE_POLL_YIELD)
-#define IXGBE_QV_USER_PEND (IXGBE_QV_STATE_POLL | IXGBE_QV_STATE_POLL_YIELD)
- spinlock_t lock;
+ atomic_t state;
#endif /* CONFIG_NET_RX_BUSY_POLL */
/* for dynamic allocation of rings associated with this q_vector */
struct ixgbe_ring ring[0] ____cacheline_internodealigned_in_smp;
};
+
#ifdef CONFIG_NET_RX_BUSY_POLL
+enum ixgbe_qv_state_t {
+ IXGBE_QV_STATE_IDLE = 0,
+ IXGBE_QV_STATE_NAPI,
+ IXGBE_QV_STATE_POLL,
+ IXGBE_QV_STATE_DISABLE
+};
+
static inline void ixgbe_qv_init_lock(struct ixgbe_q_vector *q_vector)
{
-
- spin_lock_init(&q_vector->lock);
- q_vector->state = IXGBE_QV_STATE_IDLE;
+ /* reset state to idle */
+ atomic_set(&q_vector->state, IXGBE_QV_STATE_IDLE);
}
/* called from the device poll routine to get ownership of a q_vector */
static inline bool ixgbe_qv_lock_napi(struct ixgbe_q_vector *q_vector)
{
- int rc = true;
- spin_lock_bh(&q_vector->lock);
- if (q_vector->state & IXGBE_QV_LOCKED) {
- WARN_ON(q_vector->state & IXGBE_QV_STATE_NAPI);
- q_vector->state |= IXGBE_QV_STATE_NAPI_YIELD;
- rc = false;
+ int rc = atomic_cmpxchg(&q_vector->state, IXGBE_QV_STATE_IDLE,
+ IXGBE_QV_STATE_NAPI);
#ifdef BP_EXTENDED_STATS
+ if (rc != IXGBE_QV_STATE_IDLE)
q_vector->tx.ring->stats.yields++;
#endif
- } else {
- /* we don't care if someone yielded */
- q_vector->state = IXGBE_QV_STATE_NAPI;
- }
- spin_unlock_bh(&q_vector->lock);
- return rc;
+
+ return rc == IXGBE_QV_STATE_IDLE;
}
/* returns true is someone tried to get the qv while napi had it */
-static inline bool ixgbe_qv_unlock_napi(struct ixgbe_q_vector *q_vector)
+static inline void ixgbe_qv_unlock_napi(struct ixgbe_q_vector *q_vector)
{
- int rc = false;
- spin_lock_bh(&q_vector->lock);
- WARN_ON(q_vector->state & (IXGBE_QV_STATE_POLL |
- IXGBE_QV_STATE_NAPI_YIELD));
-
- if (q_vector->state & IXGBE_QV_STATE_POLL_YIELD)
- rc = true;
- /* will reset state to idle, unless QV is disabled */
- q_vector->state &= IXGBE_QV_STATE_DISABLED;
- spin_unlock_bh(&q_vector->lock);
- return rc;
+ WARN_ON(atomic_read(&q_vector->state) != IXGBE_QV_STATE_NAPI);
+
+ /* flush any outstanding Rx frames */
+ if (q_vector->napi.gro_list)
+ napi_gro_flush(&q_vector->napi, false);
+
+ /* reset state to idle */
+ atomic_set(&q_vector->state, IXGBE_QV_STATE_IDLE);
}
/* called from ixgbe_low_latency_poll() */
static inline bool ixgbe_qv_lock_poll(struct ixgbe_q_vector *q_vector)
{
- int rc = true;
- spin_lock_bh(&q_vector->lock);
- if ((q_vector->state & IXGBE_QV_LOCKED)) {
- q_vector->state |= IXGBE_QV_STATE_POLL_YIELD;
- rc = false;
+ int rc = atomic_cmpxchg(&q_vector->state, IXGBE_QV_STATE_IDLE,
+ IXGBE_QV_STATE_POLL);
#ifdef BP_EXTENDED_STATS
- q_vector->rx.ring->stats.yields++;
+ if (rc != IXGBE_QV_STATE_IDLE)
+ q_vector->tx.ring->stats.yields++;
#endif
- } else {
- /* preserve yield marks */
- q_vector->state |= IXGBE_QV_STATE_POLL;
- }
- spin_unlock_bh(&q_vector->lock);
- return rc;
+ return rc == IXGBE_QV_STATE_IDLE;
}
/* returns true if someone tried to get the qv while it was locked */
-static inline bool ixgbe_qv_unlock_poll(struct ixgbe_q_vector *q_vector)
+static inline void ixgbe_qv_unlock_poll(struct ixgbe_q_vector *q_vector)
{
- int rc = false;
- spin_lock_bh(&q_vector->lock);
- WARN_ON(q_vector->state & (IXGBE_QV_STATE_NAPI));
-
- if (q_vector->state & IXGBE_QV_STATE_POLL_YIELD)
- rc = true;
- /* will reset state to idle, unless QV is disabled */
- q_vector->state &= IXGBE_QV_STATE_DISABLED;
- spin_unlock_bh(&q_vector->lock);
- return rc;
+ WARN_ON(atomic_read(&q_vector->state) != IXGBE_QV_STATE_POLL);
+
+ /* reset state to idle */
+ atomic_set(&q_vector->state, IXGBE_QV_STATE_IDLE);
}
/* true if a socket is polling, even if it did not get the lock */
static inline bool ixgbe_qv_busy_polling(struct ixgbe_q_vector *q_vector)
{
- WARN_ON(!(q_vector->state & IXGBE_QV_OWNED));
- return q_vector->state & IXGBE_QV_USER_PEND;
+ return atomic_read(&q_vector->state) == IXGBE_QV_STATE_POLL;
}
/* false if QV is currently owned */
static inline bool ixgbe_qv_disable(struct ixgbe_q_vector *q_vector)
{
- int rc = true;
- spin_lock_bh(&q_vector->lock);
- if (q_vector->state & IXGBE_QV_OWNED)
- rc = false;
- q_vector->state |= IXGBE_QV_STATE_DISABLED;
- spin_unlock_bh(&q_vector->lock);
-
- return rc;
+ int rc = atomic_cmpxchg(&q_vector->state, IXGBE_QV_STATE_IDLE,
+ IXGBE_QV_STATE_DISABLE);
+
+ return rc == IXGBE_QV_STATE_IDLE;
}
#else /* CONFIG_NET_RX_BUSY_POLL */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
index ae36fd6..7ecd99c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
@@ -807,6 +807,11 @@ static int ixgbe_alloc_q_vector(struct ixgbe_adapter *adapter,
ixgbe_poll, 64);
napi_hash_add(&q_vector->napi);
+#ifdef CONFIG_NET_RX_BUSY_POLL
+ /* initialize busy poll */
+ atomic_set(&q_vector->state, IXGBE_QV_STATE_DISABLE);
+
+#endif
/* tie q_vector and adapter together */
adapter->q_vector[v_idx] = q_vector;
q_vector->adapter = adapter;
--
1.9.3
^ permalink raw reply related
* [net-next 02/11] ixgbevf: Resolve missing-field-initializers warnings
From: Jeff Kirsher @ 2014-09-12 12:33 UTC (permalink / raw)
To: davem; +Cc: Mark Rustad, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <1410525231-19851-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Mark Rustad <mark.d.rustad@intel.com>
Resolve missing-field-initializers warnings by using
designated initialization.
Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbevf/ethtool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ixgbevf/ethtool.c b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
index d420f12..cc0e5b7 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ethtool.c
@@ -523,7 +523,7 @@ static const struct ixgbevf_reg_test reg_test_vf[] = {
{ IXGBE_VFTDBAL(0), 2, PATTERN_TEST, 0xFFFFFF80, 0xFFFFFFFF },
{ IXGBE_VFTDBAH(0), 2, PATTERN_TEST, 0xFFFFFFFF, 0xFFFFFFFF },
{ IXGBE_VFTDLEN(0), 2, PATTERN_TEST, 0x000FFF80, 0x000FFF80 },
- { 0, 0, 0, 0 }
+ { .reg = 0 }
};
static const u32 register_test_patterns[] = {
--
1.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox