Re: [Intel-wired-lan] [PATCH] e1000e: changed some expensive calls of udelay to usleep_range
On Thu 2017-09-07 22:19:47, Brown, Aaron F wrote: > > From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On > > Behalf Of Pavel Machek > > Sent: Monday, September 4, 2017 9:26 AM > > To: Matthew Tan > > Cc: michael.kardo...@nxp.com; Williams, Mitch A > > ; linux-ker...@vger.kernel.org; > > john.ronc...@intel.com; intel-wired-...@lists.osuosl.org; > > netdev@vger.kernel.org > > Subject: Re: [Intel-wired-lan] [PATCH] e1000e: changed some expensive calls > > of udelay to usleep_range > > > > Hi! > > > > > @@ -183,7 +183,7 @@ s32 e1000e_read_phy_reg_mdic(struct e1000_hw > > *hw, u32 offset, u16 *data) > > >* reading duplicate data in the next MDIC transaction. > > >*/ > > > if (hw->mac.type == e1000_pch2lan) > > > - udelay(100); > > > + usleep_range(90, 100); > > > > > > return 0; > > > } > > > > Can you explain why shortening the delay is acceptable here? > > Maybe it's not. > > This patch is causing speed / duplex tests to fail on several of my test > systems. Specifically a Lenova laptop with an 82577 and a NUC with an i218 > (though that does not mean it is limited to those or that it's not related to > the individual link partner.) > Ok, this should be quite easy to verify -- just adjust all the ranges to be >= original ones. Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
RE: [Intel-wired-lan] [PATCH] e1000e: changed some expensive calls of udelay to usleep_range
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On > Behalf Of Pavel Machek > Sent: Monday, September 4, 2017 9:26 AM > To: Matthew Tan > Cc: michael.kardo...@nxp.com; Williams, Mitch A > ; linux-ker...@vger.kernel.org; > john.ronc...@intel.com; intel-wired-...@lists.osuosl.org; > netdev@vger.kernel.org > Subject: Re: [Intel-wired-lan] [PATCH] e1000e: changed some expensive calls > of udelay to usleep_range > > Hi! > > > @@ -183,7 +183,7 @@ s32 e1000e_read_phy_reg_mdic(struct e1000_hw > *hw, u32 offset, u16 *data) > > * reading duplicate data in the next MDIC transaction. > > */ > > if (hw->mac.type == e1000_pch2lan) > > - udelay(100); > > + usleep_range(90, 100); > > > > return 0; > > } > > Can you explain why shortening the delay is acceptable here? Maybe it's not. This patch is causing speed / duplex tests to fail on several of my test systems. Specifically a Lenova laptop with an 82577 and a NUC with an i218 (though that does not mean it is limited to those or that it's not related to the individual link partner.) In both cases they "blow up" when attempting to link at 10 Mb with a Call Trace on the console / log and a watchdog: BUG: soft lockup - CPU#X stuck appearing in the current login session. The simplest way to produce the crash is simply load the interface and attempt to bring it up with the link partner configured to 10 Mb half (force or autoneg) or forced to 10 Mb full (autoneg to 10 Mb full did not blow up.) The problem is also appearing at some, but not all 100 Mb speeds and duplexes. 100 Mb also crashes, apparently with the same pattern. With the link partner set to 100 Mb half Autoneg on, 100 Mb half forced and 100 Mb full forced the system crashes, 100 Full autoneg seems to work fine. The call trace repeats every 30 seconds, is captured in the log and looks a lot like this: Sep 7 14:17:10 u1459 kernel: watchdog: BUG: soft lockup - CPU#2 stuck for 22s! [kworker/2:0:23] Sep 7 14:17:10 u1459 kernel: Modules linked in: e1000e ptp pps_core ip6table_filter ip6_tables ebtable_nat ebtables nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack libcrc32c ipt_REJECT nf_reject_ipv4 xt_CHECKSUM iptable_mangle iptable_filter ip_tables bridge stp llc nfsd lockd grace nfs_acl auth_rpcgss sunrpc autofs4 ipv6 crc_ccitt dm_mirror dm_region_hash dm_log vhost_net vhost tap tun kvm_intel kvm irqbypass uinput joydev ax88179_178a usbnet mii iTCO_wdt iTCO_vendor_support sg i2c_i801 lpc_ich mfd_core xhci_pci snd_hda_codec_realtek xhci_hcd snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore dm_mod(E) dax(E) ext4(E) jbd2(E) mbcache(E) sd_mod(E) ahci(E) libahci(E) i915(E) drm_kms_helper(E) drm(E) fb_sys_fops(E) Sep 7 14:17:10 u1459 kernel: sysimgblt(E) sysfillrect(E) syscopyarea(E) i2c_algo_bit(E) i2c_core(E) iosf_mbi(E) video(E) Sep 7 14:17:10 u1459 kernel: CPU: 2 PID: 23 Comm: kworker/2:0 Tainted: G EL 4.13.0-rc6_next-queue_dev-queue_regress #38 Sep 7 14:17:10 u1459 kernel: Hardware name: /NUC5i5MYBE, BIOS MYBDWi5v.86A.0030.2016.0527.1657 05/27/2016 Sep 7 14:17:10 u1459 kernel: Workqueue: events linkwatch_event Sep 7 14:17:10 u1459 kernel: task: 88024cf84680 task.stack: c9d44000 Sep 7 14:17:10 u1459 kernel: RIP: 0010:queued_spin_lock_slowpath+0x56/0x1d0 Sep 7 14:17:10 u1459 kernel: RSP: 0018:c9d478c8 EFLAGS: 0202 ORIG_RAX: ff10 Sep 7 14:17:10 u1459 kernel: RAX: 0101 RBX: 880239fe8000 RCX: 0101 Sep 7 14:17:10 u1459 kernel: RDX: c9d47948 RSI: 0001 RDI: 880239feb1a0 Sep 7 14:17:10 u1459 kernel: RBP: c9d47968 R08: 0001 R09: 88024ce181a4 Sep 7 14:17:10 u1459 kernel: R10: R11: R12: 880239fe8840 Sep 7 14:17:10 u1459 kernel: R13: 88024ce180e4 R14: 88024ce180e4 R15: c9d47a48 Sep 7 14:17:10 u1459 kernel: FS: () GS:880255d0() knlGS: Sep 7 14:17:10 u1459 kernel: CS: 0010 DS: ES: CR0: 80050033 Sep 7 14:17:10 u1459 kernel: CR2: 7ffd4c3579b8 CR3: 01c09000 CR4: 003406e0 Sep 7 14:17:10 u1459 kernel: DR0: DR1: DR2: Sep 7 14:17:10 u1459 kernel: DR3: DR6: fffe0ff0 DR7: 0400 Sep 7 14:17:10 u1459 kernel: Call Trace: Sep 7 14:17:10 u1459 kernel: ? netlink_broadcast_filtered+0x15f/0x1a0 Sep 7 14:17:10 u1459 kernel: _raw_spin_lock+0x21/0x30 Sep 7 14:17:10 u1459 kernel: e1000e_get_stats64+0x2b/0x140 [e1000e] Sep 7 14:17:10 u1459 kernel: dev_get_stats+0x3d/0xc0 Sep 7 14:17:10
Re: [Intel-wired-lan] [PATCH] e1000e: changed some expensive calls of udelay to usleep_range
Dear Matthew, On 08/23/17 17:59, Matthew Tan wrote: Calls to udelay are not preemtable by userspace so userspace applications experience a large (~200us) latency when running on core 0. Instead usleep_range can be used to be more friendly to userspace since it is preemtable. This is due to udelay using busy-wait loops while usleep_rang uses hrtimers instead. It is recommended to use udelay when the delay is <10us since at that precision overhead of usleep_range hrtimer setup causes issues. However, the replaced calls are for 50us and 100us so this should not be not an issue. Is there a reason for indenting the paragraph. (I guess, you did `git show` or `git log -p` and copied the message?) Anyway, please remove whitespace in the front, if there is no reason. Also, it looks like you ran benchmarks, so what is the delay for userspace applications with your changes? Signed-off-by: Matthew Tan --- drivers/net/ethernet/intel/e1000e/phy.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) […] Kind regards, Paul
Re: [Intel-wired-lan] [PATCH] e1000e: changed some expensive calls of udelay to usleep_range
On 8/23/2017 18:59, Matthew Tan wrote:
Calls to udelay are not preemtable by userspace so userspace
applications experience a large (~200us) latency when running on core
0. Instead usleep_range can be used to be more friendly to userspace
since it is preemtable. This is due to udelay using busy-wait loops
while usleep_rang uses hrtimers instead. It is recommended to use
udelay when the delay is <10us since at that precision overhead of
usleep_range hrtimer setup causes issues. However, the replaced calls
are for 50us and 100us so this should not be not an issue.
Signed-off-by: Matthew Tan
---
drivers/net/ethernet/intel/e1000e/phy.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/phy.c
b/drivers/net/ethernet/intel/e1000e/phy.c
index de13aea..e318fdc 100644
--- a/drivers/net/ethernet/intel/e1000e/phy.c
+++ b/drivers/net/ethernet/intel/e1000e/phy.c
@@ -158,7 +158,7 @@ s32 e1000e_read_phy_reg_mdic(struct e1000_hw *hw, u32
offset, u16 *data)
* the lower time out
*/
for (i = 0; i < (E1000_GEN_POLL_TIMEOUT * 3); i++) {
- udelay(50);
+ usleep_range(40, 60);
mdic = er32(MDIC);
if (mdic & E1000_MDIC_READY)
break;
@@ -183,7 +183,7 @@ s32 e1000e_read_phy_reg_mdic(struct e1000_hw *hw, u32
offset, u16 *data)
* reading duplicate data in the next MDIC transaction.
*/
if (hw->mac.type == e1000_pch2lan)
- udelay(100);
+ usleep_range(90, 100);
return 0;
}
@@ -222,7 +222,7 @@ s32 e1000e_write_phy_reg_mdic(struct e1000_hw *hw, u32
offset, u16 data)
* the lower time out
*/
for (i = 0; i < (E1000_GEN_POLL_TIMEOUT * 3); i++) {
- udelay(50);
+ usleep_range(40, 60);
mdic = er32(MDIC);
if (mdic & E1000_MDIC_READY)
break;
@@ -246,7 +246,7 @@ s32 e1000e_write_phy_reg_mdic(struct e1000_hw *hw, u32
offset, u16 data)
* reading duplicate data in the next MDIC transaction.
*/
if (hw->mac.type == e1000_pch2lan)
- udelay(100);
+ usleep_range(90, 110);
return 0;
}
Reasonable. Do you have any open bug or other reference describe this
problem?
