Message ID | 20190830004635.24863-1-olteanv@gmail.com |
---|---|
Headers | show |
Series | tc-taprio offload for SJA1105 DSA | expand |
On Fri, 30 Aug 2019 03:46:20 +0300, Vladimir Oltean wrote: > - Configuring the switch over SPI cannot apparently be done from this > ndo_setup_tc callback because it runs in atomic context. I also have > some downstream patches to offload tc clsact matchall with mirred > action, but in that case it looks like the atomic context restriction > does not apply. This sounds really surprising ndo_setup_tc should always be allowed to sleep. Can the taprio limitation be lifted somehow?
Hi Jakub, On Fri, 30 Aug 2019 at 04:21, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Fri, 30 Aug 2019 03:46:20 +0300, Vladimir Oltean wrote: > > - Configuring the switch over SPI cannot apparently be done from this > > ndo_setup_tc callback because it runs in atomic context. I also have > > some downstream patches to offload tc clsact matchall with mirred > > action, but in that case it looks like the atomic context restriction > > does not apply. > > This sounds really surprising ndo_setup_tc should always be allowed to > sleep. Can the taprio limitation be lifted somehow? I need to get more familiar with the taprio internal data structures. I think you're suggesting to get those updated to a consistent state while under spin_lock_bh(qdisc_lock(sch)), then call ndo_setup_tc from outside that critical section? Also, I just noticed that I introduced a bug in taprio_disable_offload with my reference counting addition. The qdisc can't just pass stack memory to the driver, now that it's allowing it to keep it. So definitely the patch needs more refactoring. Thanks, -Vladimir
On Fri, 30 Aug 2019 13:11:11 +0300, Vladimir Oltean wrote: > On Fri, 30 Aug 2019 at 04:21, Jakub Kicinski wrote: > > On Fri, 30 Aug 2019 03:46:20 +0300, Vladimir Oltean wrote: > > > - Configuring the switch over SPI cannot apparently be done from this > > > ndo_setup_tc callback because it runs in atomic context. I also have > > > some downstream patches to offload tc clsact matchall with mirred > > > action, but in that case it looks like the atomic context restriction > > > does not apply. > > > > This sounds really surprising ndo_setup_tc should always be allowed to > > sleep. Can the taprio limitation be lifted somehow? > > I need to get more familiar with the taprio internal data structures. > I think you're suggesting to get those updated to a consistent state > while under spin_lock_bh(qdisc_lock(sch)), then call ndo_setup_tc from > outside that critical section? I'm not 100% sure how taprio handles locking TBH, it just seems naive that HW callback will not need to sleep, so the kernel should make sure that callback can sleep. Otherwise we'll end up with 3/4 of drivers implementing some async work routine... Sorry, I know that's quite general and not that helpful. > Also, I just noticed that I introduced a bug in taprio_disable_offload > with my reference counting addition. The qdisc can't just pass stack > memory to the driver, now that it's allowing it to keep it. So > definitely the patch needs more refactoring. Ah, a slight deja vu, I think someone else has done it in the past :)
On Sat, 31 Aug 2019 at 01:29, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Fri, 30 Aug 2019 13:11:11 +0300, Vladimir Oltean wrote: > > On Fri, 30 Aug 2019 at 04:21, Jakub Kicinski wrote: > > > On Fri, 30 Aug 2019 03:46:20 +0300, Vladimir Oltean wrote: > > > > - Configuring the switch over SPI cannot apparently be done from this > > > > ndo_setup_tc callback because it runs in atomic context. I also have > > > > some downstream patches to offload tc clsact matchall with mirred > > > > action, but in that case it looks like the atomic context restriction > > > > does not apply. > > > > > > This sounds really surprising ndo_setup_tc should always be allowed to > > > sleep. Can the taprio limitation be lifted somehow? > > > > I need to get more familiar with the taprio internal data structures. > > I think you're suggesting to get those updated to a consistent state > > while under spin_lock_bh(qdisc_lock(sch)), then call ndo_setup_tc from > > outside that critical section? > > I'm not 100% sure how taprio handles locking TBH, it just seems naive > that HW callback will not need to sleep, so the kernel should make sure > that callback can sleep. Otherwise we'll end up with 3/4 of drivers > implementing some async work routine... > > Sorry, I know that's quite general and not that helpful. > > > Also, I just noticed that I introduced a bug in taprio_disable_offload > > with my reference counting addition. The qdisc can't just pass stack > > memory to the driver, now that it's allowing it to keep it. So > > definitely the patch needs more refactoring. > > Ah, a slight deja vu, I think someone else has done it in the past :) Ok, I think I managed to move the ndo_setup_tc callback outside of atomic context and nothing broke so far... Any other comments or should I go ahead and send a first proper patchset? Thanks, -Vladimir
> I'm not 100% sure how taprio handles locking TBH, it just seems naive > that HW callback will not need to sleep, so the kernel should make sure > that callback can sleep. Otherwise we'll end up with 3/4 of drivers > implementing some async work routine... Hi Jakub I suspect this is because until recently, all such devices were on a PCI bus, for some other form of memory mapped device. It is only recently with DSA becoming popular, that we need to handle devices on the end of other sorts of bus, be is MDIO, SPI or i2c. Andrew