Message ID | 1442952096-31430-1-git-send-email-nhorman@redhat.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 22 Sep 2015 16:01:36 -0400 Neil Horman <nhorman@redhat.com> wrote: > + clear_bit(NAPI_STATE_NPSVC, &n->state); > + > } why introduce extra line here? > + /* > + * If we set this bit but see that it has already been set, > + * that indicates that napi has been disabled and we need > + * to abort this operation > + */ > + > + if(test_and_set_bit(NAPI_STATE_NPSVC, &napi->state)) And why introduce line after comment before code. My preference is to have comment as close to code as possible. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 22, 2015 at 03:09:20PM -0700, Stephen Hemminger wrote: > On Tue, 22 Sep 2015 16:01:36 -0400 > Neil Horman <nhorman@redhat.com> wrote: > > > + clear_bit(NAPI_STATE_NPSVC, &n->state); > > + > > } > why introduce extra line here? > > > + /* > > + * If we set this bit but see that it has already been set, > > + * that indicates that napi has been disabled and we need > > + * to abort this operation > > + */ > > + > > + if(test_and_set_bit(NAPI_STATE_NPSVC, &napi->state)) > > And why introduce line after comment before code. > My preference is to have comment as close to code as possible. Mine is to add spaces, as I feel the code is more readable if its a bit more spread out. > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Neil Horman <nhorman@redhat.com> Date: Tue, 22 Sep 2015 16:01:36 -0400 > - set_bit(NAPI_STATE_NPSVC, &napi->state); > + /* > + * If we set this bit but see that it has already been set, > + * that indicates that napi has been disabled and we need > + * to abort this operation > + */ > + > + if(test_and_set_bit(NAPI_STATE_NPSVC, &napi->state)) > + goto out; > Networking comments should be styled: /* Like * this. */ Also you need a space after the "if" and before the openning parenthesis. And like Stephen I think the empty line between the comment and the if() statement is superfluous and eats up precious vertical space on the screen :-) Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index b791405..48becac 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -507,6 +507,8 @@ static inline void napi_enable(struct napi_struct *n) BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state)); smp_mb__before_atomic(); clear_bit(NAPI_STATE_SCHED, &n->state); + clear_bit(NAPI_STATE_NPSVC, &n->state); + } #ifdef CONFIG_SMP diff --git a/net/core/dev.c b/net/core/dev.c index ee0d628..b6b01bf 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4723,6 +4723,8 @@ void napi_disable(struct napi_struct *n) while (test_and_set_bit(NAPI_STATE_SCHED, &n->state)) msleep(1); + while (test_and_set_bit(NAPI_STATE_NPSVC, &n->state)) + msleep(1); hrtimer_cancel(&n->timer); diff --git a/net/core/netpoll.c b/net/core/netpoll.c index 6aa3db8..9312b66 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -142,7 +142,7 @@ static void queue_process(struct work_struct *work) */ static int poll_one_napi(struct napi_struct *napi, int budget) { - int work; + int work = 0; /* net_rx_action's ->poll() invocations and our's are * synchronized by this test which is only made while @@ -151,7 +151,14 @@ static int poll_one_napi(struct napi_struct *napi, int budget) if (!test_bit(NAPI_STATE_SCHED, &napi->state)) return budget; - set_bit(NAPI_STATE_NPSVC, &napi->state); + /* + * If we set this bit but see that it has already been set, + * that indicates that napi has been disabled and we need + * to abort this operation + */ + + if(test_and_set_bit(NAPI_STATE_NPSVC, &napi->state)) + goto out; work = napi->poll(napi, budget); WARN_ONCE(work > budget, "%pF exceeded budget in poll\n", napi->poll); @@ -159,6 +166,7 @@ static int poll_one_napi(struct napi_struct *napi, int budget) clear_bit(NAPI_STATE_NPSVC, &napi->state); +out: return budget - work; }