| Message ID | 20160912221327.5610.74333.stgit@john-Precision-Tower-5810 |
|---|---|
| State | Changes Requested |
| Delegated to: | Jeff Kirsher |
| Headers | show |
On Mon, Sep 12, 2016 at 3:13 PM, John Fastabend <john.fastabend@gmail.com> wrote: > The BQL API does not reference the sk_buff nor does the driver need to > reference the sk_buff to calculate the length of a transmitted frame. > This patch removes an sk_buff reference from the xmit irq path and > also allows packets sent from XDP to use BQL. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > drivers/net/ethernet/intel/e1000/e1000_main.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c > index f42129d..62a7f8d 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c > @@ -3882,11 +3882,8 @@ static bool e1000_clean_tx_irq(struct e1000_adapter *adapter, > if (cleaned) { > total_tx_packets += buffer_info->segs; > total_tx_bytes += buffer_info->bytecount; > - if (buffer_info->skb) { > - bytes_compl += buffer_info->skb->len; > - pkts_compl++; > - } > - > + bytes_compl += buffer_info->length; > + pkts_compl++; > } > e1000_unmap_and_free_tx_resource(adapter, buffer_info); > tx_desc->upper.data = 0; Actually it might be worth looking into why we have two different stats for tracking bytecount and segs. From what I can tell the pkts_compl value is never actually used. The function doesn't even use the value so it is just wasted cycles. And as far as the bytes go the accounting would be more accurate if you were to use bytecount instead of buffer_info->skb->len. You would just need to update the xmit function to use that on the other side so that they match. - Alex
On Mon, Sep 12, 2016 at 8:00 PM, Alexander Duyck <alexander.duyck@gmail.com> wrote: > On Mon, Sep 12, 2016 at 3:13 PM, John Fastabend > <john.fastabend@gmail.com> wrote: >> The BQL API does not reference the sk_buff nor does the driver need to >> reference the sk_buff to calculate the length of a transmitted frame. >> This patch removes an sk_buff reference from the xmit irq path and >> also allows packets sent from XDP to use BQL. >> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >> --- >> drivers/net/ethernet/intel/e1000/e1000_main.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c >> index f42129d..62a7f8d 100644 >> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >> @@ -3882,11 +3882,8 @@ static bool e1000_clean_tx_irq(struct e1000_adapter *adapter, >> if (cleaned) { >> total_tx_packets += buffer_info->segs; >> total_tx_bytes += buffer_info->bytecount; >> - if (buffer_info->skb) { >> - bytes_compl += buffer_info->skb->len; >> - pkts_compl++; >> - } >> - >> + bytes_compl += buffer_info->length; >> + pkts_compl++; >> } >> e1000_unmap_and_free_tx_resource(adapter, buffer_info); >> tx_desc->upper.data = 0; > > Actually it might be worth looking into why we have two different > stats for tracking bytecount and segs. From what I can tell the > pkts_compl value is never actually used. The function doesn't even > use the value so it is just wasted cycles. And as far as the bytes go Transmit flow steering which I posted and is pending on some testing uses the pkt count BQL to track inflight packets. > the accounting would be more accurate if you were to use bytecount > instead of buffer_info->skb->len. You would just need to update the > xmit function to use that on the other side so that they match. > > - Alex
On Mon, Sep 12, 2016 at 9:25 PM, Tom Herbert <tom@herbertland.com> wrote: > On Mon, Sep 12, 2016 at 8:00 PM, Alexander Duyck > <alexander.duyck@gmail.com> wrote: >> On Mon, Sep 12, 2016 at 3:13 PM, John Fastabend >> <john.fastabend@gmail.com> wrote: >>> The BQL API does not reference the sk_buff nor does the driver need to >>> reference the sk_buff to calculate the length of a transmitted frame. >>> This patch removes an sk_buff reference from the xmit irq path and >>> also allows packets sent from XDP to use BQL. >>> >>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >>> --- >>> drivers/net/ethernet/intel/e1000/e1000_main.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c >>> index f42129d..62a7f8d 100644 >>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c >>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c >>> @@ -3882,11 +3882,8 @@ static bool e1000_clean_tx_irq(struct e1000_adapter *adapter, >>> if (cleaned) { >>> total_tx_packets += buffer_info->segs; >>> total_tx_bytes += buffer_info->bytecount; >>> - if (buffer_info->skb) { >>> - bytes_compl += buffer_info->skb->len; >>> - pkts_compl++; >>> - } >>> - >>> + bytes_compl += buffer_info->length; >>> + pkts_compl++; >>> } >>> e1000_unmap_and_free_tx_resource(adapter, buffer_info); >>> tx_desc->upper.data = 0; >> >> Actually it might be worth looking into why we have two different >> stats for tracking bytecount and segs. From what I can tell the >> pkts_compl value is never actually used. The function doesn't even >> use the value so it is just wasted cycles. And as far as the bytes go > > Transmit flow steering which I posted and is pending on some testing > uses the pkt count BQL to track inflight packets. > >> the accounting would be more accurate if you were to use bytecount >> instead of buffer_info->skb->len. You would just need to update the >> xmit function to use that on the other side so that they match. Okay, that makes sense. But as I was saying we might be better off using the segs and bytecount values instead of the skb->len in the xmit and cleanup paths to get more accurate accounting for the total bytes/packets coming and going from the interface. That way we can avoid any significant change in behavior between TSO and GSO. - Alex
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On > Behalf Of John Fastabend > Sent: Monday, September 12, 2016 3:13 PM > To: bblanco@plumgrid.com; john.fastabend@gmail.com; > alexei.starovoitov@gmail.com; Kirsher, Jeffrey T > <jeffrey.t.kirsher@intel.com>; brouer@redhat.com; davem@davemloft.net > Cc: xiyou.wangcong@gmail.com; intel-wired-lan@lists.osuosl.org; > u9012063@gmail.com; netdev@vger.kernel.org > Subject: [Intel-wired-lan] [net-next PATCH v3 1/3] e1000: track BQL bytes > regardless of skb or not > > The BQL API does not reference the sk_buff nor does the driver need to > reference the sk_buff to calculate the length of a transmitted frame. > This patch removes an sk_buff reference from the xmit irq path and > also allows packets sent from XDP to use BQL. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > drivers/net/ethernet/intel/e1000/e1000_main.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) This patch is causing all my e1000 adapters to fail a simple ftp session with really slow response (hashing on) followed by adapter resets. I have seen this on 4 different e1000 nics now, an 82543GC, 82544GC, 82546EB and an 82545GM. On a few occasions I get a splat captured to dmesg. Here is an example: -------------------------------- ------------[ cut here ]------------ WARNING: CPU: 1 PID: 0 at net/sched/sch_generic.c:316 dev_watchdog+0x1c2/0x1d0 NETDEV WATCHDOG: eth1 (e1000): transmit queue 0 timed out Modules linked in: e1000 e100 nfsd lockd grace nfs_acl auth_rpcgss sunrpc autofs 4 ipv6 crc_ccitt p4_clockmod dm_mirror dm_region_hash dm_log uinput sg serio_raw dcdbas mii i2c_piix4 i2c_core cfi_probe gen_probe cfi_util scb2_flash dm_mod(E) ext4(E) mbcache(E) jbd2(E) sd_mod(E) sr_mod(E) cdrom(E) aacraid(E) pata_acpi(E) ata_generic(E) pata_serverworks(E) [last unloaded: e1000] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G E 4.8.0-rc6_next-queue_dev -queue_b0b5ade #8 Hardware name: Dell Computer Corporation PowerEdge 2650 /0D5995, BIO S A21 10/05/2006 00000000 c06724fb c0b6038a c0b6038a 0000013c c04596d5 c0b647ac f3d2fed0 00000000 c0b6038a 0000013c c08bff52 c08bff52 00000009 f2922000 00000000 f318cf00 0001e788 c045979b 00000009 00000000 f3d2feb8 c0b647ac f3d2fed0 Call Trace: [<c06724fb>] ? dump_stack+0x47/0x6c [<c04596d5>] ? __warn+0x105/0x120 [<c08bff52>] ? dev_watchdog+0x1c2/0x1d0 [<c08bff52>] ? dev_watchdog+0x1c2/0x1d0 [<c045979b>] ? warn_slowpath_fmt+0x3b/0x40 [<c08bff52>] ? dev_watchdog+0x1c2/0x1d0 [<c04bb71b>] ? call_timer_fn+0x3b/0x140 [<c08bfd90>] ? dev_trans_start+0x60/0x60 [<c04bc1ab>] ? expire_timers+0x9b/0x110 [<c04bc471>] ? run_timer_softirq+0xd1/0x260 [<c089a1e0>] ? net_tx_action+0xe0/0x1a0 [<c04b8597>] ? rcu_process_callbacks+0x47/0xe0 [<c045e518>] ? __do_softirq+0xc8/0x280 [<c045e450>] ? irq_exit+0x90/0x90 [<c041dbd2>] ? do_softirq_own_stack+0x22/0x30 <IRQ> [<c045e445>] ? irq_exit+0x85/0x90 [<c0440f80>] ? smp_apic_timer_interrupt+0x30/0x40 [<c095f44d>] ? apic_timer_interrupt+0x2d/0x34 [<c04251cc>] ? default_idle+0x1c/0xd0 [<c04ce222>] ? __tick_nohz_idle_enter+0x92/0x140 [<c0424cc6>] ? arch_cpu_idle+0x6/0x10 [<c0495ebd>] ? cpuidle_idle_call+0x7d/0xe0 [<c0496075>] ? cpu_idle_loop+0x155/0x210 [<c04404a5>] ? start_secondary+0xb5/0xe0 ---[ end trace fa448b49f7848a42 ]--- e1000 0000:02:06.0 eth1: Reset adapter e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX/TX e1000 0000:02:06.0 eth1: Reset adapter e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX/TX e1000 0000:02:06.0 eth1: Reset adapter e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX/TX e1000 0000:02:06.0 eth1: Reset adapter e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX/TX e1000 0000:02:06.0 eth1: Reset adapter e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX/TX e1000 0000:02:06.0 eth1: Reset adapter e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX/TX e1000 0000:02:06.0 eth1: Reset adapter e1000: eth1 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: RX/TX
On Wed, Sep 14, 2016 at 11:57:24PM +0000, Brown, Aaron F wrote: > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On > > Behalf Of John Fastabend > > Sent: Monday, September 12, 2016 3:13 PM > > To: bblanco@plumgrid.com; john.fastabend@gmail.com; > > alexei.starovoitov@gmail.com; Kirsher, Jeffrey T > > <jeffrey.t.kirsher@intel.com>; brouer@redhat.com; davem@davemloft.net > > Cc: xiyou.wangcong@gmail.com; intel-wired-lan@lists.osuosl.org; > > u9012063@gmail.com; netdev@vger.kernel.org > > Subject: [Intel-wired-lan] [net-next PATCH v3 1/3] e1000: track BQL bytes > > regardless of skb or not > > > > The BQL API does not reference the sk_buff nor does the driver need to > > reference the sk_buff to calculate the length of a transmitted frame. > > This patch removes an sk_buff reference from the xmit irq path and > > also allows packets sent from XDP to use BQL. > > > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > > --- > > drivers/net/ethernet/intel/e1000/e1000_main.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > This patch is causing all my e1000 adapters to fail a simple ftp session with really slow response (hashing on) followed by adapter resets. I have seen this on 4 different e1000 nics now, an 82543GC, 82544GC, 82546EB and an 82545GM. On a few occasions I get a splat captured to dmesg. Here is an example: > -------------------------------- > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 0 at net/sched/sch_generic.c:316 dev_watchdog+0x1c2/0x1d0 > NETDEV WATCHDOG: eth1 (e1000): transmit queue 0 timed out Thanks a lot for the tests! Really appreciate it.
On 16-09-14 05:43 PM, Alexei Starovoitov wrote: > On Wed, Sep 14, 2016 at 11:57:24PM +0000, Brown, Aaron F wrote: >>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On >>> Behalf Of John Fastabend >>> Sent: Monday, September 12, 2016 3:13 PM >>> To: bblanco@plumgrid.com; john.fastabend@gmail.com; >>> alexei.starovoitov@gmail.com; Kirsher, Jeffrey T >>> <jeffrey.t.kirsher@intel.com>; brouer@redhat.com; davem@davemloft.net >>> Cc: xiyou.wangcong@gmail.com; intel-wired-lan@lists.osuosl.org; >>> u9012063@gmail.com; netdev@vger.kernel.org >>> Subject: [Intel-wired-lan] [net-next PATCH v3 1/3] e1000: track BQL bytes >>> regardless of skb or not >>> >>> The BQL API does not reference the sk_buff nor does the driver need to >>> reference the sk_buff to calculate the length of a transmitted frame. >>> This patch removes an sk_buff reference from the xmit irq path and >>> also allows packets sent from XDP to use BQL. >>> >>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >>> --- >>> drivers/net/ethernet/intel/e1000/e1000_main.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> This patch is causing all my e1000 adapters to fail a simple ftp session with really slow response (hashing on) followed by adapter resets. I have seen this on 4 different e1000 nics now, an 82543GC, 82544GC, 82546EB and an 82545GM. On a few occasions I get a splat captured to dmesg. Here is an example: >> -------------------------------- >> ------------[ cut here ]------------ >> WARNING: CPU: 1 PID: 0 at net/sched/sch_generic.c:316 dev_watchdog+0x1c2/0x1d0 >> NETDEV WATCHDOG: eth1 (e1000): transmit queue 0 timed out > > Thanks a lot for the tests! Really appreciate it. > Thanks. Jeff, please drop the series for now obviously this wont work. It needs some work.
> > ------------[ cut here ]------------ > > WARNING: CPU: 1 PID: 0 at net/sched/sch_generic.c:316 > dev_watchdog+0x1c2/0x1d0 > > NETDEV WATCHDOG: eth1 (e1000): transmit queue 0 timed out > > Thanks a lot for the tests! Really appreciate it. np, I needed to get my old compatibility systems back in running order anyway.
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c index f42129d..62a7f8d 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_main.c +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c @@ -3882,11 +3882,8 @@ static bool e1000_clean_tx_irq(struct e1000_adapter *adapter, if (cleaned) { total_tx_packets += buffer_info->segs; total_tx_bytes += buffer_info->bytecount; - if (buffer_info->skb) { - bytes_compl += buffer_info->skb->len; - pkts_compl++; - } - + bytes_compl += buffer_info->length; + pkts_compl++; } e1000_unmap_and_free_tx_resource(adapter, buffer_info); tx_desc->upper.data = 0;
The BQL API does not reference the sk_buff nor does the driver need to reference the sk_buff to calculate the length of a transmitted frame. This patch removes an sk_buff reference from the xmit irq path and also allows packets sent from XDP to use BQL. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- drivers/net/ethernet/intel/e1000/e1000_main.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)