On Tue, 2017-02-28 at 09:20 -0800, Alexander Duyck wrote:

On Mon, Feb 27, 2017 at 6:21 AM, Eric Dumazet [off-list ref] wrote:

quoted

+bool napi_schedule_prep(struct napi_struct *n)
+{
+       unsigned long val, new;
+
+       do {
+               val = READ_ONCE(n->state);
+               if (unlikely(val & NAPIF_STATE_DISABLE))
+                       return false;
+               new = val | NAPIF_STATE_SCHED;
+               if (unlikely(val & NAPIF_STATE_SCHED))
+                       new |= NAPIF_STATE_MISSED;

You might want to consider just using a combination AND, divide,
multiply, and OR to avoid having to have any conditional branches
being added due to this code path.  Basically the logic would look
like:

    new = val | NAPIF_STATE_SCHED;
    new |= (val & NAPIF_STATE_SCHED) / NAPIF_STATE_SCHED * NAPIF_STATE_MISSED;

In assembler that all ends up getting translated out to AND, SHL, OR.
You avoid the branching, or MOV/OR/TEST/CMOV type code you would end
up with otherwise.

Sure, I can try to optimize this a bit ;)

quoted

+       } while (cmpxchg(&n->state, val, new) != val);
+
+       if (unlikely(val & NAPIF_STATE_MISSED))
+               __napi_schedule(n);
+
        return true;
 }

If you rescheduled napi should you really be returning true?  Seems
like you should be returning "!(val & NAPIF_STATE_MISSED)" to try to
avoid letting this occur again.

Good idea.

Hmm... you mean that many drivers test napi_complete_done() return
value ?

;)

Thanks !