netdev - RE: [PATCH v2 net] bnx2x: Fix busy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <B5657A6538887040AD3A81F1008BEC63CF5B1E@AVMB1.qlogic.org>
Date:	Wed, 15 Apr 2015 04:05:00 +0000
From:	Yuval Mintz <Yuval.Mintz@...gic.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>, "bind@...s.net" <bind@...s.net>,
	"peter@...leysoftware.com" <peter@...leysoftware.com>,
	Ariel Elior <Ariel.Elior@...gic.com>,
	"Willem de Bruijn" <willemb@...gle.com>
Subject: RE: [PATCH v2 net] bnx2x: Fix busy_poll vs netpoll

>  #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?

> +               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?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html