Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
On Thu, Dec 03, 2020 at 08:01:40PM +0200, Vladimir Oltean wrote: > On Thu, Dec 03, 2020 at 06:53:20PM +0100, Oleksij Rempel wrote: > > It is possible to poll it more frequently, but it make no reals sense > > on this low power devices. > > Frankly I thought you understood the implications of periodic polling > and you're ok with them, I added polling to read out small counters to avoid overflow. > just wanting to have _something_. Having something is good, but making it good is better :D > But fine, > welcome to my world, happy to have you onboard... > > > What kind of options do we have? > > https://www.spinics.net/lists/netdev/msg703774.html > https://www.spinics.net/lists/netdev/msg704370.html > > Unfortunately I've been absolutely snowed under with work lately. I hope > to be able to come back to that during the weekend or something like that. Ok, so the strategy is to fix the original issue. Sound good. For now I'll resend this patches without accessing mdio regs from the stats64 callback. It will give initial play ground, so we can see what else should be done for DSA specific use case. Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
On Thu, Dec 03, 2020 at 06:53:20PM +0100, Oleksij Rempel wrote: > It is possible to poll it more frequently, but it make no reals sense > on this low power devices. Frankly I thought you understood the implications of periodic polling and you're ok with them, just wanting to have _something_. But fine, welcome to my world, happy to have you onboard... > What kind of options do we have? https://www.spinics.net/lists/netdev/msg703774.html https://www.spinics.net/lists/netdev/msg704370.html Unfortunately I've been absolutely snowed under with work lately. I hope to be able to come back to that during the weekend or something like that.
Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
On Thu, 3 Dec 2020 18:53:20 +0100 Oleksij Rempel wrote: > On Thu, Dec 03, 2020 at 08:35:17AM -0800, Jakub Kicinski wrote: > > On Thu, 3 Dec 2020 09:50:11 +0100 Oleksij Rempel wrote: > > > @Jakub, > > > > > > > You can't take sleeping locks from .ndo_get_stats64. > > > > > > > > Also regmap may sleep? > > > > > > > > + ret = regmap_read(priv->regmap, reg, ); > > > > > > Yes. And underling layer is mdio bus which is by default sleeping as > > > well. > > > > > > > Am I missing something? > > > > > > In this log, the ar9331_get_stats64() was never called from atomic or > > > irq context. Why it should not be sleeping? > > > > You missed some long discussions about this within last week on netdev. > > Also Documentation/networking/statistics.rst. > > > > To answer your direct question - try: > > > > # cat /proc/net/dev > > > > procfs iterates over devices while holding only an RCU read lock. > > Now i can reproduce it :) > > [33683.199864] BUG: sleeping function called from invalid context at > kernel/locking/mutex.c:935 > [33683.210737] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 593, > name: cat > [33683.216796] INFO: lockdep is turned off. > [33683.222972] CPU: 0 PID: 593 Comm: cat Not tainted > 5.10.0-rc3-ar9331-00733-gff7090915bb7-dirty #28 > [33683.231743] Stack : 808f 80885ffc 820eba5c d4a19200 > 8098 819a93c8 > [33683.240093] 80980ca7 80d43358 804ee1f4 8098 0002 800afe08 > 820eba08 d4a19200 > [33683.247181] 8089ffb0 820ebfe0 > > [33683.257767] 820ebab4 77bbfdc0 00fae587 77e859a0 8098 8000 > 8099 > [33683.266107] 804ee1f4 8098 0002 8200f750 8097ca9c d4a19200 > 000859df 0001 > [33683.274529] ... > [33683.275626] Call Trace: > [33683.280156] [<80069ce0>] show_stack+0x9c/0x140 > [33683.283200] [<800afe08>] ___might_sleep+0x220/0x244 > [33683.290441] [<8073c030>] __mutex_lock+0x70/0x374 > [33683.293651] [<8073c360>] mutex_lock_nested+0x2c/0x38 > [33683.300793] [<804ee1f4>] ar9331_read_stats+0x34/0x834 > [33683.304441] [<804eea34>] ar9331_get_stats64+0x40/0x394 > [33683.311797] [<80526584>] dev_get_stats+0x58/0xfc > [33683.315013] [<805657bc>] dev_seq_printf_stats+0x44/0x228 > [33683.322476] [<805659e8>] dev_seq_show+0x48/0x50 > [33683.325601] [<8021dd28>] seq_read_iter+0x3d8/0x4d0 > [33683.332585] [<8021df60>] seq_read+0x140/0x198 > [33683.335532] [<8026f950>] proc_reg_read+0xe4/0xf8 > [33683.342397] [<801f0840>] vfs_read+0xc8/0x1a8 > [33683.345260] [<801f0b7c>] ksys_read+0x9c/0xfc > [33683.352056] [<80071aa4>] syscall_common+0x34/0x58 > > Hm.. There is no way i can guarantee that underlying mdio system is > not using mutexes. So, i can't read stats directly from HW within > ar9331_get_stats64(), only driver internal storage can be used. It is possible > to poll it more frequently, but it make no reals sense on this low power > devices. > > What kind of options do we have? Vladimir has been looking at solving this, I'll let him answer with his latest thoughts.
Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
On Thu, Dec 03, 2020 at 08:35:17AM -0800, Jakub Kicinski wrote: > On Thu, 3 Dec 2020 09:50:11 +0100 Oleksij Rempel wrote: > > @Jakub, > > > > > You can't take sleeping locks from .ndo_get_stats64. > > > > > > Also regmap may sleep? > > > > > > + ret = regmap_read(priv->regmap, reg, ); > > > > Yes. And underling layer is mdio bus which is by default sleeping as > > well. > > > > > Am I missing something? > > > > In this log, the ar9331_get_stats64() was never called from atomic or > > irq context. Why it should not be sleeping? > > You missed some long discussions about this within last week on netdev. > Also Documentation/networking/statistics.rst. > > To answer your direct question - try: > > # cat /proc/net/dev > > procfs iterates over devices while holding only an RCU read lock. Now i can reproduce it :) [33683.199864] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:935 [33683.210737] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 593, name: cat [33683.216796] INFO: lockdep is turned off. [33683.222972] CPU: 0 PID: 593 Comm: cat Not tainted 5.10.0-rc3-ar9331-00733-gff7090915bb7-dirty #28 [33683.231743] Stack : 808f 80885ffc 820eba5c d4a19200 8098 819a93c8 [33683.240093] 80980ca7 80d43358 804ee1f4 8098 0002 800afe08 820eba08 d4a19200 [33683.247181] 8089ffb0 820ebfe0 [33683.257767] 820ebab4 77bbfdc0 00fae587 77e859a0 8098 8000 8099 [33683.266107] 804ee1f4 8098 0002 8200f750 8097ca9c d4a19200 000859df 0001 [33683.274529] ... [33683.275626] Call Trace: [33683.280156] [<80069ce0>] show_stack+0x9c/0x140 [33683.283200] [<800afe08>] ___might_sleep+0x220/0x244 [33683.290441] [<8073c030>] __mutex_lock+0x70/0x374 [33683.293651] [<8073c360>] mutex_lock_nested+0x2c/0x38 [33683.300793] [<804ee1f4>] ar9331_read_stats+0x34/0x834 [33683.304441] [<804eea34>] ar9331_get_stats64+0x40/0x394 [33683.311797] [<80526584>] dev_get_stats+0x58/0xfc [33683.315013] [<805657bc>] dev_seq_printf_stats+0x44/0x228 [33683.322476] [<805659e8>] dev_seq_show+0x48/0x50 [33683.325601] [<8021dd28>] seq_read_iter+0x3d8/0x4d0 [33683.332585] [<8021df60>] seq_read+0x140/0x198 [33683.335532] [<8026f950>] proc_reg_read+0xe4/0xf8 [33683.342397] [<801f0840>] vfs_read+0xc8/0x1a8 [33683.345260] [<801f0b7c>] ksys_read+0x9c/0xfc [33683.352056] [<80071aa4>] syscall_common+0x34/0x58 Hm.. There is no way i can guarantee that underlying mdio system is not using mutexes. So, i can't read stats directly from HW within ar9331_get_stats64(), only driver internal storage can be used. It is possible to poll it more frequently, but it make no reals sense on this low power devices. What kind of options do we have? Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
On Thu, 3 Dec 2020 09:50:11 +0100 Oleksij Rempel wrote: > @Jakub, > > > You can't take sleeping locks from .ndo_get_stats64. > > > > Also regmap may sleep? > > > > + ret = regmap_read(priv->regmap, reg, ); > > Yes. And underling layer is mdio bus which is by default sleeping as > well. > > > Am I missing something? > > In this log, the ar9331_get_stats64() was never called from atomic or > irq context. Why it should not be sleeping? You missed some long discussions about this within last week on netdev. Also Documentation/networking/statistics.rst. To answer your direct question - try: # cat /proc/net/dev procfs iterates over devices while holding only an RCU read lock.
Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
> @Andrew > > > You could update the stats here, after the interface is down. You then > > know the stats are actually up to date and correct! > > stats are automatically read on __dev_notify_flags(), for example here: > > [ 11.049289] [<80069ce0>] show_stack+0x9c/0x140 > [ 11.053651] [<804eea2c>] ar9331_get_stats64+0x38/0x394 > [ 11.058820] [<80526584>] dev_get_stats+0x58/0xfc > [ 11.063385] [<805428c8>] rtnl_fill_stats+0x6c/0x14c > [ 11.068293] [<8054703c>] rtnl_fill_ifinfo+0x548/0xcec > [ 11.073274] [<8054a4d4>] rtmsg_ifinfo_build_skb+0xbc/0x134 > [ 11.078799] [<8054a5d4>] rtmsg_ifinfo_event+0x4c/0x84 > [ 11.083782] [<8054a6c8>] rtmsg_ifinfo+0x2c/0x38 > [ 11.088378] [<80534380>] __dev_notify_flags+0x50/0xd8 > [ 11.093365] [<80534ca0>] dev_change_flags+0x60/0x80 > [ 11.098273] [<80c13fa4>] ic_close_devs+0xcc/0xdc > [ 11.102810] [<80c15200>] ip_auto_config+0x1144/0x11e4 > [ 11.107847] [<80060f14>] do_one_initcall+0x110/0x2b4 > [ 11.112871] [<80bf31bc>] kernel_init_freeable+0x220/0x258 > [ 11.118248] [<80739a2c>] kernel_init+0x24/0x11c > [ 11.122707] [<8006306c>] ret_from_kernel_thread+0x14/0x1c > > Do we really need an extra read within the ar9331 driver? Ah, did not know that. Nice, Somebody solved this for all drivers at once. Andrew
Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
Hello Jakub and Andrew, > Ah, I missed the v3 (like most reviewers it seems :)). No problem, I moved your replies from v2 tread to this mail. On Wed, Dec 02, 2020 at 10:42:07AM -0800, Jakub Kicinski wrote: > On Wed, 2 Dec 2020 15:09:04 +0100 Oleksij Rempel wrote: > > Add stats support for the ar9331 switch. > > > > Signed-off-by: Oleksij Rempel I added dump_stack() to the stats64 function, so we can see how it is used. @Andrew > You could update the stats here, after the interface is down. You then > know the stats are actually up to date and correct! stats are automatically read on __dev_notify_flags(), for example here: [ 11.049289] [<80069ce0>] show_stack+0x9c/0x140 [ 11.053651] [<804eea2c>] ar9331_get_stats64+0x38/0x394 [ 11.058820] [<80526584>] dev_get_stats+0x58/0xfc [ 11.063385] [<805428c8>] rtnl_fill_stats+0x6c/0x14c [ 11.068293] [<8054703c>] rtnl_fill_ifinfo+0x548/0xcec [ 11.073274] [<8054a4d4>] rtmsg_ifinfo_build_skb+0xbc/0x134 [ 11.078799] [<8054a5d4>] rtmsg_ifinfo_event+0x4c/0x84 [ 11.083782] [<8054a6c8>] rtmsg_ifinfo+0x2c/0x38 [ 11.088378] [<80534380>] __dev_notify_flags+0x50/0xd8 [ 11.093365] [<80534ca0>] dev_change_flags+0x60/0x80 [ 11.098273] [<80c13fa4>] ic_close_devs+0xcc/0xdc [ 11.102810] [<80c15200>] ip_auto_config+0x1144/0x11e4 [ 11.107847] [<80060f14>] do_one_initcall+0x110/0x2b4 [ 11.112871] [<80bf31bc>] kernel_init_freeable+0x220/0x258 [ 11.118248] [<80739a2c>] kernel_init+0x24/0x11c [ 11.122707] [<8006306c>] ret_from_kernel_thread+0x14/0x1c Do we really need an extra read within the ar9331 driver? @Jakub, > You can't take sleeping locks from .ndo_get_stats64. > > Also regmap may sleep? > > + ret = regmap_read(priv->regmap, reg, ); Yes. And underling layer is mdio bus which is by default sleeping as well. > Am I missing something? In this log, the ar9331_get_stats64() was never called from atomic or irq context. Why it should not be sleeping? [0.00] Linux version 5.10.0-rc3-ar9331-00733-gff7090915bb7-dirty (ptxdist@ptxdist) (mips-softfloat-linux-gnu-gcc (OSELAS.Toolchain-2020.08.0 10-20200822) 10.2.1 20200822, GNU ld (GNU Binutils) 2.35) #28 PREEMPT 2020-12-03T08:20:00+00:00 [0.00] printk: bootconsole [early0] enabled [0.00] CPU0 revision is: 00019374 (MIPS 24Kc) [0.00] MIPS: machine is DPTechnics DPT-Module [0.00] SoC: Atheros AR9330 rev 1 [0.00] Primary instruction cache 64kB, VIPT, 4-way, linesize 32 bytes. [0.00] Primary data cache 32kB, 4-way, VIPT, cache aliases, linesize 32 bytes [0.00] Zone ranges: [0.00] Normal [mem 0x-0x1fff] [0.00] Movable zone start for each node [0.00] Early memory node ranges [0.00] node 0: [mem 0x-0x03ff] [0.00] node 0: [mem 0x8000-0x83ff] [0.00] Initmem setup node 0 [mem 0x-0x83ff] [0.00] On node 0 totalpages: 16384 [0.00] Normal zone: 1024 pages used for memmap [0.00] Normal zone: 0 pages reserved [0.00] Normal zone: 16384 pages, LIFO batch:3 [0.00] pcpu-alloc: s0 r0 d32768 u32768 alloc=1*32768 [0.00] pcpu-alloc: [0] 0 [0.00] Built 1 zonelists, mobility grouping on. Total pages: 15360 [0.00] Kernel command line: ip=dhcp root=/dev/nfs nfsroot=192.168.1.100:/home/none/nfsroot/som9331-board,v3,tcp [0.00] Dentry cache hash table entries: 8192 (order: 3, 32768 bytes, linear) [0.00] Inode-cache hash table entries: 4096 (order: 2, 16384 bytes, linear) [0.00] Writing ErrCtl register= [0.00] Readback ErrCtl register= [0.00] mem auto-init: stack:off, heap alloc:off, heap free:off [0.00] Memory: 41424K/65536K available (7045K kernel code, 2555K rwdata, 2240K rodata, 1336K init, 6706K bss, 24112K reserved, 0K cma-reserved) [0.00] random: get_random_u32 called from cache_random_seq_create+0xa0/0x198 with crng_init=0 [0.00] SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1 [0.00] ftrace: allocating 22486 entries in 44 pages [0.00] ftrace: allocated 44 pages with 3 groups [0.00] Running RCU self tests [0.00] rcu: Preemptible hierarchical RCU implementation. [0.00] rcu: RCU lockdep checking is enabled. [0.00] Trampoline variant of Tasks RCU enabled. [0.00] Rude variant of Tasks RCU enabled. [0.00] Tracing variant of Tasks RCU enabled. [0.00] rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies. [0.00] NR_IRQS: 51 [0.00] CPU clock: 400.000 MHz [0.00] clocksource: MIPS: mask: 0x max_cycles: 0x, max_idle_ns: 9556302233 ns [0.21] sched_clock: 32 bits at 200MHz, resolution 5ns, wraps every 10737418237ns [0.008709] Lock dependency validator: Copyright (c) 2006 Red Hat,
Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
On Wed, 2 Dec 2020 15:09:04 +0100 Oleksij Rempel wrote: > Add stats support for the ar9331 switch. > > Signed-off-by: Oleksij Rempel Ah, I missed the v3 (like most reviewers it seems :)). The sleeping in ndo_get_stats64 question applies.
Re: [PATCH v3 net-next 2/2] net: dsa: qca: ar9331: export stats64
On 12/2/2020 6:09 AM, Oleksij Rempel wrote: > Add stats support for the ar9331 switch. > > Signed-off-by: Oleksij Rempel Reviewed-by: Florian Fainelli -- Florian
