net: Add __napi_sycnhronize() to sync with napi poll

Note: I use msleep_interruptible(1); just like napi_disable(). However I'm not too happy that the "hot" loop that results of a pending signal here will spin without even a cpu_relax ... what do you guys think would be the best way to handle this ?

Quoted 5 lines · Benjamin Herrenschmidt

Note: I use msleep_interruptible(1); just like napi_disable(). However I'm not too happy that the "hot" loop that results of a pending signal here will spin without even a cpu_relax ... what do you guys think would be the best way to handle this ?

Well since the loop does not check signals at all, it should just use msleep.

Granted the process will end up in the D state and contribute to the load average. But if this loop executes long enough for that to be noticed then we've got bigger problems to worry about.

Quoted 10 lines

Quoted 5 lines · Benjamin Herrenschmidt

Note: I use msleep_interruptible(1); just like napi_disable(). However I'm not too happy that the "hot" loop that results of a pending signal here will spin without even a cpu_relax ... what do you guys think would be the best way to handle this ?

Well since the loop does not check signals at all, it should just use msleep.

Quoted 4 lines

Granted the process will end up in the D state and contribute to the load average. But if this loop executes long enough for that to be noticed then we've got bigger problems to worry about.

If Dave & Stephen agree, I'll send a patch changing napi_disable() too then.

net: Add __napi_sycnhronize() to sync with napi poll

The EMAC driver which needs to handle multiple devices with one NAPI instance implements its own per-channel disable bit. However, when setting such a bit, it needs to synchronize with the poller (that is make sure that any pending poller instance has completed, or is started late enough to see that disable bit).

This implements a low level __napi_synchronize() function to acheive that. The underscores are to emphasis the low level aspect of it and to discourage driver writers who don't know what they are doing to use it (to please DaveM :-)

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

(Use correct address for Stephen this time)

If the approach is accepted, I would like to have this merged now so the EMAC patch to make it work again can follow :-)

Note: I use msleep_interruptible(1); just like napi_disable(). However I'm not too happy that the "hot" loop that results of a pending signal here will spin without even a cpu_relax ... what do you guys think would be the best way to handle this ?

So this is really just like synchronize_irq()? Using msleep is bogus because you want to spin, you are only waiting for a softirq on the other cpu to finish. If you wait for a whole millisecond and sleep that is far longer than the napi routine should take.

You could even optimize it like synchronize_irq() for the non-SMP case.

net: Add __napi_synchronize() to sync with napi poll

The EMAC driver which needs to handle multiple devices with one NAPI instance implements its own per-channel disable bit. However, when setting such a bit, it needs to synchronize with the poller (that is make sure that any pending poller instance has completed, or is started late enough to see that disable bit).

This implements a low level __napi_synchronize() function to acheive that. The underscores are to emphasis the low level aspect of it and to discourage driver writers who don't know what they are doing to use it (to please DaveM :-)

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

Switched to do a cpu_relax() spin instead and only on SMP. Not that we have an smp_mb() in there which synchronize_irq() lacks. I believe this is a bug in synchronize_irq() which I'll handle separately.

Note: unfortunately, Jeff already picked up the EMAC patch without waiting for this to be sorted out (oops...). So if you agree with this patch, it would be nice to have it go in quickly or maybe via Jeff's tree to avoid breakage ? Not terribly important tho.

Quoted 4 lines

Note: unfortunately, Jeff already picked up the EMAC patch without waiting for this to be sorted out (oops...). So if you agree with this patch, it would be nice to have it go in quickly or maybe via Jeff's tree to avoid breakage ? Not terribly important tho.

Sorry, I thought that was the way everybody was headed. With the driver broken /anyway/, I just sorta threw it into the pile of fixes.

It's upstream now, so let me know if you want to revert or move forward from here...

Please don't use double underscore, for this function name. There is no reason to not make it a normal API call.

The sky2 fix I am working on will use napi_synchronize as well.

include/linux/netdevice.h +18 -0

@@ -407,6 +407,24 @@ static inline void napi_enable(struct na
 	clear_bit(NAPI_STATE_SCHED, &n->state);
 }
 
+#ifdef CONFIG_SMP
+/**
+ *	napi_synchronize - wait until NAPI is not running
+ *	@n: napi context
+ *
+ * Wait until NAPI is done being scheduled on this context.
+ * Any outstanding processing completes but
+ * does not disable future activations.
+ */
+static inline void napi_synchronize(const struct napi_struct *n)
+{
+	while (test_bit(NAPI_STATE_SCHED, &n->state))
+		msleep(1);
+}
+#else
+# define napi_synchronize(n)	barrier()
+#endif
+
 /*
  *	The DEVICE structure.
  *	Actually, this whole structure is a big mistake.  It mixes I/O

net: Add __napi_synchronize() to sync with napi poll

The EMAC driver which needs to handle multiple devices with one NAPI instance implements its own per-channel disable bit. However, when setting such a bit, it needs to synchronize with the poller (that is make sure that any pending poller instance has completed, or is started late enough to see that disable bit).

This implements a low level __napi_synchronize() function to acheive that. The underscores are to emphasis the low level aspect of it and to discourage driver writers who don't know what they are doing to use it (to please DaveM :-)

Quoted 8 lines · Benjamin Herrenschmidt

net: Add __napi_synchronize() to sync with napi poll

The EMAC driver which needs to handle multiple devices with one NAPI instance implements its own per-channel disable bit. However, when setting such a bit, it needs to synchronize with the poller (that is make sure that any pending poller instance has completed, or is started late enough to see that disable bit).

Quoted 4 lines

This implements a low level __napi_synchronize() function to acheive that. The underscores are to emphasis the low level aspect of it and to discourage driver writers who don't know what they are doing to use it (to please DaveM :-)

Erm.. your commit log calls it __napi_synchronize still.

net: Add __napi_synchronize() to sync with napi poll

The EMAC driver which needs to handle multiple devices with one NAPI instance implements its own per-channel disable bit. However, when setting such a bit, it needs to synchronize with the poller (that is make sure that any pending poller instance has completed, or is started late enough to see that disable bit).

This implements a low level __napi_synchronize() function to acheive that. The underscores are to emphasis the low level aspect of it and to discourage driver writers who don't know what they are doing to use it (to please DaveM :-)

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

Back to msleep() since it fits my need well and that's what you used as well. Note that the smp_mb() will turn into barrier() on non-SMP.

I'll send a separate patch to fix EMAC to use the non __ version.

Index: linux-work/include/linux/netdevice.h
===================================================================

include/linux/netdevice.h +17 -0

@@ -394,6 +394,23 @@ static inline void napi_disable(struct n
 }
 
 /**
+ *	napi_synchronize - synchronize with a concurrent poll
+ *	@n: napi context
+ *
+ * Synchronizes with a concurrent poll. Not to be used in normal
+ * drivers, mostly useful if you end up with multiple interfaces
+ * on one NAPI instance. This must be called from task context.
+ */
+static inline void napi_synchronize(struct napi_struct *n)
+{
+	smp_mb();
+#ifdef CONFIG_SMP
+	while (test_bit(NAPI_STATE_SCHED, &n->state))
+		msleep(1);
+#endif /* CONFIG_SMP */
+}
+
+/**
  *	napi_enable - enable NAPI scheduling
  *	@n: napi context
  *

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

k, trying again, leaving the actual patch below. In the above patch description, you still call it __napi_synchronize. In the patch below,
it's napi_syncronize.

I was just trying to point out to whomever commits this patch that the actual description should probably be fixed.

This implements a low level __napi_synchronize() function to acheive that. The underscores are to emphasis the low level aspect of it and to discourage driver writers who don't know what they are doing to use it (to please DaveM :-)

The EMAC driver "fix" was merged by mistake before the dust had settled on the new napi synchronize interface (and before it got merged). The final version of that function is spelled without underscores.

Signed-off-by Benjamin Herrenschmidt <[email protected]>