| Message ID | 1521988761-30344-4-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> Subject: [PATCH v7 3/7] bnx2x: Replace doorbell barrier() with wmb() > > barrier() doesn't guarantee memory writes to be observed by the hardware on > all architectures. barrier() only tells compiler not to move this code > with respect to other read/writes. > > If memory write needs to be observed by the HW, wmb() is the right choice. The wmb() is there (a couple of lines above). Your modification adds an unnecessary fence which would hurt high pps scenarios. The memory writes which the HW needs to observe are the buffer descriptors, not the producer update message. The producer is written to the HW, and exists on the stack. The barrier() is there to prevent the compiler from mixing the order of the prod update message preparation and writing it to the host. A possible alternative would be to move the existing wmb() to where the barrier() is, achieving both goals, although in the existing design each barrier has a distinct purpose. The comment location is misleading, though. Thanks, Ariel
Hi Ariel, On 3/29/2018 5:17 AM, Elior, Ariel wrote: >> Subject: [PATCH v7 3/7] bnx2x: Replace doorbell barrier() with wmb() >> >> barrier() doesn't guarantee memory writes to be observed by the hardware on >> all architectures. barrier() only tells compiler not to move this code >> with respect to other read/writes. >> >> If memory write needs to be observed by the HW, wmb() is the right choice. > The wmb() is there (a couple of lines above). Your modification adds an > unnecessary fence which would hurt high pps scenarios. The memory > writes which the HW needs to observe are the buffer descriptors, not the > producer update message. The producer is written to the HW, and exists > on the stack. The barrier() is there to prevent the compiler from mixing the > order of the prod update message preparation and writing it to the host. > A possible alternative would be to move the existing wmb() to where > the barrier() is, achieving both goals, although in the existing design each > barrier has a distinct purpose. The comment location is misleading, though. I was told that barrier() is there to guarantee that HW is observing the memory write before writel(). I reacted to this and changed barrier() to wmb() following the old directions. You are saying that this not true. Since then, Linus gave us direction not to have wmb() in front of writel() as writel() already has memory-IO guarantee. https://www.mail-archive.com/netdev@vger.kernel.org/msg225806.html I'll be doing one more pass to remove wmb() before writel() soon. Please review that carefully. Intel drivers use wmb() as a substitute for smp_wmb(). So, we can't always assume that you can remove all wmb() in front of writel() as the write barrier seems to serve dual purpose. Please help me getting this right on the next version. Sinan
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c index d7c98e8..0f86f18 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c @@ -4153,7 +4153,8 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev) wmb(); txdata->tx_db.data.prod += nbd; - barrier(); + /* make sure descriptor update is observed by HW */ + wmb(); DOORBELL(bp, txdata->cid, txdata->tx_db.raw); diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c index 1e33abd..39af4f8 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c @@ -2591,7 +2591,8 @@ static int bnx2x_run_loopback(struct bnx2x *bp, int loopback_mode) wmb(); txdata->tx_db.data.prod += 2; - barrier(); + /* make sure descriptor update is observed by the HW */ + wmb(); DOORBELL(bp, txdata->cid, txdata->tx_db.raw); mmiowb();
barrier() doesn't guarantee memory writes to be observed by the hardware on all architectures. barrier() only tells compiler not to move this code with respect to other read/writes. If memory write needs to be observed by the HW, wmb() is the right choice. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 3 ++- drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)