On Wed, 2015-04-15 at 04:05 +0000, Yuval Mintz wrote: > > #ifdef CONFIG_NET_RX_BUSY_POLL > > -static inline void bnx2x_fp_init_lock(struct bnx2x_fastpath *fp) > > + > > +enum bnx2x_fp_state { > > + BNX2X_STATE_FP_NAPI = BIT(0), /* NAPI handler owns the queue */ > > + > > + BNX2X_STATE_FP_NAPI_REQ_BIT = 1, /* NAPI would like to own the > > queue */ > > + BNX2X_STATE_FP_NAPI_REQ = BIT(1), > > + > > + BNX2X_STATE_FP_POLL_BIT = 2, > > + BNX2X_STATE_FP_POLL = BIT(2), /* busy_poll owns the queue */ > > + > > + BNX2X_STATE_FP_DISABLE_BIT = 3, /* queue is dismantled */ > > +}; > > ... > > > /* called from the device poll routine to get ownership of a FP */ > > static inline bool bnx2x_fp_lock_napi(struct bnx2x_fastpath *fp) > > { > > + unsigned long prev, old = READ_ONCE(fp->busy_poll_state); > > + > > + while (1) { > > + switch (old) { > > + case BNX2X_STATE_FP_POLL: > > + /* make sure bnx2x_fp_lock_poll() wont starve us */ > > + set_bit(BNX2X_STATE_FP_NAPI_REQ_BIT, > > + &fp->busy_poll_state); > > + /* fallthrough */ > > + case BNX2X_STATE_FP_POLL | BNX2X_STATE_FP_NAPI_REQ: > > + return false; > > + default: > > + break; > > + } > > + prev = cmpxchg(&fp->busy_poll_state, old, > > BNX2X_STATE_FP_NAPI); > > Wouldn't this override the disabled status? Shouldn't we return 'false' > if BNX2X_STATE_FP_DISABLE_BIT is set?
My reasoning is that at this point BNX2X_STATE_FP_DISABLE_BIT cannot be set. BNX2X_STATE_FP_DISABLE_BIT is set only after a call to napi_disable() napi_disable(&bnx2x_fp(bp, i, napi)); while (!bnx2x_fp_ll_disable(&bp->fp[i])) usleep_range(1000, 2000); It is important to assume this, otherwise bnx2x_fp_unlock_napi() could not be a simple write, but would need a locked operation. Also note that napi_disable() itself sets NAPI_STATE_DISABLE so that napi_poll() can shortcut the napi->poll() at one point and break a potential infinite loop under rx pressure. set_bit(NAPI_STATE_DISABLE, &n->state); > > > + if (unlikely(prev != old)) { > > + old = prev; > > + continue; > > + } > > + return true; > > } > > } > > BTW, this looks quite generic - isn't it possible to take it out of the > driver and push it into the networking core, uniforming it in the process? Yes, this is the plan I have in mind, once net-next is opened again ;) Thanks ! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html