* [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations @ 2010-03-09 16:55 Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Stanislaw Gruszka 2010-03-10 15:59 ` [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations Eilon Greenstein 0 siblings, 2 replies; 11+ messages in thread From: Stanislaw Gruszka @ 2010-03-09 16:55 UTC (permalink / raw) To: netdev Cc: Vladislav Zolotarov, Eilon Greenstein, Michael Chan, Stanislaw Gruszka Since we want to keep ordering of write to fp->bd_tx_cons and netif_tx_queue_stopped(txq), what is read of txq->state, we have to use general memory barrier. Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- drivers/net/bnx2x_main.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c index ed785a3..9fc0f6a 100644 --- a/drivers/net/bnx2x_main.c +++ b/drivers/net/bnx2x_main.c @@ -963,7 +963,7 @@ static int bnx2x_tx_int(struct bnx2x_fastpath *fp) * start_xmit() will miss it and cause the queue to be stopped * forever. */ - smp_wmb(); + smp_mb(); /* TBD need a thresh? */ if (unlikely(netif_tx_queue_stopped(txq))) { -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] bnx2x: remove not necessary compiler barrier 2010-03-09 16:55 [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations Stanislaw Gruszka @ 2010-03-09 16:55 ` Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Stanislaw Gruszka 2010-03-10 16:00 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Eilon Greenstein 2010-03-10 15:59 ` [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations Eilon Greenstein 1 sibling, 2 replies; 11+ messages in thread From: Stanislaw Gruszka @ 2010-03-09 16:55 UTC (permalink / raw) To: netdev Cc: Vladislav Zolotarov, Eilon Greenstein, Michael Chan, Stanislaw Gruszka Access to fp->tx_bd_prod is protected by __netif_tx_lock, so we do not need any barrier for that. Update of fp->tx_bd_cons in bnx2x_tx_int() is not protected by lock, but barrier() nor smb_mb() in bnx2x_tx_avail() not guarantee we will see values that is written on other cpu. Ordering issues between netif_tx_stop_queue(), netif_tx_queue_stopped(), fp->tx_bd_cons = bd_cons and bnx2x_tx_avail() are already handled by smp_mb() in bnx2x_tx_int() and bnx2x_start_xmit(). Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- drivers/net/bnx2x_main.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c index 9fc0f6a..ae62b67 100644 --- a/drivers/net/bnx2x_main.c +++ b/drivers/net/bnx2x_main.c @@ -893,7 +893,6 @@ static inline u16 bnx2x_tx_avail(struct bnx2x_fastpath *fp) u16 prod; u16 cons; - barrier(); /* Tell compiler that prod and cons can change */ prod = fp->tx_bd_prod; cons = fp->tx_bd_cons; -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true 2010-03-09 16:55 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Stanislaw Gruszka @ 2010-03-09 16:55 ` Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 4/4] bnx2x: merge common code when stopping queue Stanislaw Gruszka 2010-03-10 16:01 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Eilon Greenstein 2010-03-10 16:00 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Eilon Greenstein 1 sibling, 2 replies; 11+ messages in thread From: Stanislaw Gruszka @ 2010-03-09 16:55 UTC (permalink / raw) To: netdev Cc: Vladislav Zolotarov, Eilon Greenstein, Michael Chan, Stanislaw Gruszka Access to fp->tx_bp_prod is protected by __netif_tx_lock, smp_mb() is not needed for that. Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- drivers/net/bnx2x_main.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c index ae62b67..6c042a7 100644 --- a/drivers/net/bnx2x_main.c +++ b/drivers/net/bnx2x_main.c @@ -11428,9 +11428,12 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev) if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) { netif_tx_stop_queue(txq); - /* We want bnx2x_tx_int to "see" the updated tx_bd_prod - if we put Tx into XOFF state. */ + + /* paired memory barrier is in bnx2x_tx_int(), we have to keep + * ordering of set_bit() in netif_tx_stop_queue() and read of + * fp->bd_tx_cons */ smp_mb(); + fp->eth_q_stats.driver_xoff++; if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3) netif_tx_wake_queue(txq); -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] bnx2x: merge common code when stopping queue 2010-03-09 16:55 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Stanislaw Gruszka @ 2010-03-09 16:55 ` Stanislaw Gruszka 2010-03-10 16:02 ` Eilon Greenstein 2010-03-10 16:01 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Eilon Greenstein 1 sibling, 1 reply; 11+ messages in thread From: Stanislaw Gruszka @ 2010-03-09 16:55 UTC (permalink / raw) To: netdev Cc: Vladislav Zolotarov, Eilon Greenstein, Michael Chan, Stanislaw Gruszka Main reason for merge code is willingness to use memory barrier to avoid races with bnx2x_tx_int(). There is no real situation possible to trigger the races. But if we assume that bug of not stopped and full queue can happen, we should also stop queue then in the safe way. Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- I have no strong fillings that patch should be applied, if Vladislav do not want the patch, I'll will not argue. drivers/net/bnx2x_main.c | 37 ++++++++++++++++++++----------------- 1 files changed, 20 insertions(+), 17 deletions(-) diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c index 6c042a7..6751ca2 100644 --- a/drivers/net/bnx2x_main.c +++ b/drivers/net/bnx2x_main.c @@ -11176,10 +11176,9 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev) struct eth_tx_bd *tx_data_bd, *total_pkt_bd = NULL; struct eth_tx_parse_bd *pbd = NULL; u16 pkt_prod, bd_prod; - int nbd, fp_index; + int i, ret, nbd, fp_index; dma_addr_t mapping; u32 xmit_type = bnx2x_xmit_type(bp, skb); - int i; u8 hlen = 0; __le16 pkt_size = 0; @@ -11194,10 +11193,9 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev) fp = &bp->fp[fp_index]; if (unlikely(bnx2x_tx_avail(fp) < (skb_shinfo(skb)->nr_frags + 3))) { - fp->eth_q_stats.driver_xoff++; - netif_tx_stop_queue(txq); BNX2X_ERR("BUG! Tx ring full when queue awake!\n"); - return NETDEV_TX_BUSY; + ret = NETDEV_TX_BUSY; + goto stop_queue; } DP(NETIF_MSG_TX_QUEUED, "SKB: summed %x protocol %x protocol(%x,%x)" @@ -11425,22 +11423,27 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev) mmiowb(); fp->tx_bd_prod += nbd; + fp->tx_pkt++; - if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) { - netif_tx_stop_queue(txq); + ret = NETDEV_TX_OK; + if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) + goto stop_queue; - /* paired memory barrier is in bnx2x_tx_int(), we have to keep - * ordering of set_bit() in netif_tx_stop_queue() and read of - * fp->bd_tx_cons */ - smp_mb(); + return ret; - fp->eth_q_stats.driver_xoff++; - if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3) - netif_tx_wake_queue(txq); - } - fp->tx_pkt++; +stop_queue: + netif_tx_stop_queue(txq); + + /* paired memory barrier is in bnx2x_tx_int(), we have to keep + * ordering of set_bit() in netif_tx_stop_queue() and read of + * fp->bd_tx_cons */ + smp_mb(); - return NETDEV_TX_OK; + fp->eth_q_stats.driver_xoff++; + if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3) + netif_tx_wake_queue(txq); + + return ret; } /* called with rtnl_lock */ -- 1.6.2.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] bnx2x: merge common code when stopping queue 2010-03-09 16:55 ` [PATCH 4/4] bnx2x: merge common code when stopping queue Stanislaw Gruszka @ 2010-03-10 16:02 ` Eilon Greenstein 0 siblings, 0 replies; 11+ messages in thread From: Eilon Greenstein @ 2010-03-10 16:02 UTC (permalink / raw) To: Stanislaw Gruszka Cc: netdev@vger.kernel.org, Vladislav Zolotarov, Michael Chan On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote: > Main reason for merge code is willingness to use memory barrier to avoid > races with bnx2x_tx_int(). There is no real situation possible to > trigger the races. But if we assume that bug of not stopped and full > queue can happen, we should also stop queue then in the safe way. > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > --- > I have no strong fillings that patch should be applied, if Vladislav > do not want the patch, I'll will not argue. > Stanislaw, As you wrote, there is no real situation possible to trigger this race. So basically, you are changing the code from “return” on error to “go to” and in the “go to” branch you add some harmless but useless code in the form of smp_mb and testing for a possible wake again. I find the code to be less readable after this change and harder to maintain so I prefer not to include this code change. Thanks for all the changes and you participant in improving the bnx2x! Eilon > drivers/net/bnx2x_main.c | 37 ++++++++++++++++++++----------------- > 1 files changed, 20 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c > index 6c042a7..6751ca2 100644 > --- a/drivers/net/bnx2x_main.c > +++ b/drivers/net/bnx2x_main.c > @@ -11176,10 +11176,9 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev) > struct eth_tx_bd *tx_data_bd, *total_pkt_bd = NULL; > struct eth_tx_parse_bd *pbd = NULL; > u16 pkt_prod, bd_prod; > - int nbd, fp_index; > + int i, ret, nbd, fp_index; > dma_addr_t mapping; > u32 xmit_type = bnx2x_xmit_type(bp, skb); > - int i; > u8 hlen = 0; > __le16 pkt_size = 0; > > @@ -11194,10 +11193,9 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev) > fp = &bp->fp[fp_index]; > > if (unlikely(bnx2x_tx_avail(fp) < (skb_shinfo(skb)->nr_frags + 3))) { > - fp->eth_q_stats.driver_xoff++; > - netif_tx_stop_queue(txq); > BNX2X_ERR("BUG! Tx ring full when queue awake!\n"); > - return NETDEV_TX_BUSY; > + ret = NETDEV_TX_BUSY; > + goto stop_queue; > } > > DP(NETIF_MSG_TX_QUEUED, "SKB: summed %x protocol %x protocol(%x,%x)" > @@ -11425,22 +11423,27 @@ static netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev) > mmiowb(); > > fp->tx_bd_prod += nbd; > + fp->tx_pkt++; > > - if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) { > - netif_tx_stop_queue(txq); > + ret = NETDEV_TX_OK; > + if (unlikely(bnx2x_tx_avail(fp) < MAX_SKB_FRAGS + 3)) > + goto stop_queue; > > - /* paired memory barrier is in bnx2x_tx_int(), we have to keep > - * ordering of set_bit() in netif_tx_stop_queue() and read of > - * fp->bd_tx_cons */ > - smp_mb(); > + return ret; > > - fp->eth_q_stats.driver_xoff++; > - if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3) > - netif_tx_wake_queue(txq); > - } > - fp->tx_pkt++; > +stop_queue: > + netif_tx_stop_queue(txq); > + > + /* paired memory barrier is in bnx2x_tx_int(), we have to keep > + * ordering of set_bit() in netif_tx_stop_queue() and read of > + * fp->bd_tx_cons */ > + smp_mb(); > > - return NETDEV_TX_OK; > + fp->eth_q_stats.driver_xoff++; > + if (bnx2x_tx_avail(fp) >= MAX_SKB_FRAGS + 3) > + netif_tx_wake_queue(txq); > + > + return ret; > } > > /* called with rtnl_lock */ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true 2010-03-09 16:55 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 4/4] bnx2x: merge common code when stopping queue Stanislaw Gruszka @ 2010-03-10 16:01 ` Eilon Greenstein 2010-03-15 22:47 ` David Miller 1 sibling, 1 reply; 11+ messages in thread From: Eilon Greenstein @ 2010-03-10 16:01 UTC (permalink / raw) To: Stanislaw Gruszka Cc: netdev@vger.kernel.org, Vladislav Zolotarov, Michael Chan On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote: > Access to fp->tx_bp_prod is protected by __netif_tx_lock, > smp_mb() is not needed for that. > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> Signed-off-by: Eilon Greenstein <eilong@broadcom.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true 2010-03-10 16:01 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Eilon Greenstein @ 2010-03-15 22:47 ` David Miller 0 siblings, 0 replies; 11+ messages in thread From: David Miller @ 2010-03-15 22:47 UTC (permalink / raw) To: eilong; +Cc: sgruszka, netdev, vladz, mchan From: "Eilon Greenstein" <eilong@broadcom.com> Date: Wed, 10 Mar 2010 18:01:16 +0200 > On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote: >> Access to fp->tx_bp_prod is protected by __netif_tx_lock, >> smp_mb() is not needed for that. >> >> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > Signed-off-by: Eilon Greenstein <eilong@broadcom.com> Applied. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] bnx2x: remove not necessary compiler barrier 2010-03-09 16:55 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Stanislaw Gruszka @ 2010-03-10 16:00 ` Eilon Greenstein 2010-03-15 22:47 ` David Miller 1 sibling, 1 reply; 11+ messages in thread From: Eilon Greenstein @ 2010-03-10 16:00 UTC (permalink / raw) To: Stanislaw Gruszka Cc: netdev@vger.kernel.org, Vladislav Zolotarov, Michael Chan On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote: > Access to fp->tx_bd_prod is protected by __netif_tx_lock, so we do not > need any barrier for that. > > Update of fp->tx_bd_cons in bnx2x_tx_int() is not protected by lock, but > barrier() nor smb_mb() in bnx2x_tx_avail() not guarantee we will see > values that is written on other cpu. Ordering issues between > netif_tx_stop_queue(), netif_tx_queue_stopped(), fp->tx_bd_cons = bd_cons > and bnx2x_tx_avail() are already handled by smp_mb() in bnx2x_tx_int() > and bnx2x_start_xmit(). > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> Signed-off-by: Eilon Greenstein <eilong@broadcom.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] bnx2x: remove not necessary compiler barrier 2010-03-10 16:00 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Eilon Greenstein @ 2010-03-15 22:47 ` David Miller 0 siblings, 0 replies; 11+ messages in thread From: David Miller @ 2010-03-15 22:47 UTC (permalink / raw) To: eilong; +Cc: sgruszka, netdev, vladz, mchan From: "Eilon Greenstein" <eilong@broadcom.com> Date: Wed, 10 Mar 2010 18:00:57 +0200 > On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote: >> Access to fp->tx_bd_prod is protected by __netif_tx_lock, so we do not >> need any barrier for that. >> >> Update of fp->tx_bd_cons in bnx2x_tx_int() is not protected by lock, but >> barrier() nor smb_mb() in bnx2x_tx_avail() not guarantee we will see >> values that is written on other cpu. Ordering issues between >> netif_tx_stop_queue(), netif_tx_queue_stopped(), fp->tx_bd_cons = bd_cons >> and bnx2x_tx_avail() are already handled by smp_mb() in bnx2x_tx_int() >> and bnx2x_start_xmit(). >> >> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > Signed-off-by: Eilon Greenstein <eilong@broadcom.com> Applied. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations 2010-03-09 16:55 [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Stanislaw Gruszka @ 2010-03-10 15:59 ` Eilon Greenstein 2010-03-15 22:47 ` David Miller 1 sibling, 1 reply; 11+ messages in thread From: Eilon Greenstein @ 2010-03-10 15:59 UTC (permalink / raw) To: Stanislaw Gruszka Cc: netdev@vger.kernel.org, Vladislav Zolotarov, Michael Chan On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote: > Since we want to keep ordering of write to fp->bd_tx_cons and > netif_tx_queue_stopped(txq), what is read of txq->state, we have to use > general memory barrier. > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> Singed-of-by: Eilon Greenstein <eilong@broadcom.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations 2010-03-10 15:59 ` [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations Eilon Greenstein @ 2010-03-15 22:47 ` David Miller 0 siblings, 0 replies; 11+ messages in thread From: David Miller @ 2010-03-15 22:47 UTC (permalink / raw) To: eilong; +Cc: sgruszka, netdev, vladz, mchan From: "Eilon Greenstein" <eilong@broadcom.com> Date: Wed, 10 Mar 2010 17:59:44 +0200 > On Tue, 2010-03-09 at 08:55 -0800, Stanislaw Gruszka wrote: >> Since we want to keep ordering of write to fp->bd_tx_cons and >> netif_tx_queue_stopped(txq), what is read of txq->state, we have to use >> general memory barrier. >> >> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> > Singed-of-by: Eilon Greenstein <eilong@broadcom.com> Applied. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-03-15 22:47 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-03-09 16:55 [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Stanislaw Gruszka 2010-03-09 16:55 ` [PATCH 4/4] bnx2x: merge common code when stopping queue Stanislaw Gruszka 2010-03-10 16:02 ` Eilon Greenstein 2010-03-10 16:01 ` [PATCH 3/4] bnx2x: change smp_mb() comment to conform the true Eilon Greenstein 2010-03-15 22:47 ` David Miller 2010-03-10 16:00 ` [PATCH 2/4] bnx2x: remove not necessary compiler barrier Eilon Greenstein 2010-03-15 22:47 ` David Miller 2010-03-10 15:59 ` [PATCH 1/4] bnx2x: use smp_mb() to keep ordering of read write operations Eilon Greenstein 2010-03-15 22:47 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).