igb: Use dma_wmb() instead of wmb() before doorbell writes

Message ID 20180525041321.140516-1-venkateshs@google.com
State Accepted
Delegated to: Jeff Kirsher
Headers show
Series
  • igb: Use dma_wmb() instead of wmb() before doorbell writes
Related show

Commit Message

Venkatesh Srinivas May 25, 2018, 4:13 a.m.
igb writes to doorbells to post transmit and receive descriptors;
after writing descriptors to memory but before writing to doorbells,
use dma_wmb() rather than wmb(). wmb() is more heavyweight than
necessary before doorbell writes.

On x86, this avoids SFENCEs before doorbell writes in both the
tx and rx refill paths.

Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Alexander Duyck May 25, 2018, 3:56 p.m. | #1
On Thu, May 24, 2018 at 9:13 PM, Venkatesh Srinivas
<venkateshs@google.com> wrote:
> igb writes to doorbells to post transmit and receive descriptors;
> after writing descriptors to memory but before writing to doorbells,
> use dma_wmb() rather than wmb(). wmb() is more heavyweight than
> necessary before doorbell writes.
>
> On x86, this avoids SFENCEs before doorbell writes in both the
> tx and rx refill paths.
>
> Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index cce7ada89255..8aea50867a5d 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5674,7 +5674,7 @@ static int igb_tx_map(struct igb_ring *tx_ring,
>          * We also need this memory barrier to make certain all of the
>          * status bits have been updated before next_to_watch is written.
>          */
> -       wmb();
> +       dma_wmb();
>
>         /* set next_to_watch value indicating a packet is present */
>         first->next_to_watch = tx_desc;

This change I am good with. My first instinct would be to replace this
with just an smp_wmb(), however we are better off probably just using
the dma_wmb() so that it doesn't get compiled out in the non-smp
kernel case.

> @@ -8093,7 +8093,7 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)
>                  * applicable for weak-ordered memory model archs,
>                  * such as IA-64).
>                  */
> -               wmb();
> +               dma_wmb();
>                 writel(i, rx_ring->tail);
>         }
>  }

I'm not sure if this is even really needed. My understanding is that
the writel should be providing a barrier if needed already. Odds are
we can probably just drop the wmb() and update the comment that goes
with it.

Thanks.

- Alex
Brown, Aaron F June 5, 2018, 11:15 p.m. | #2
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On
> Behalf Of Alexander Duyck
> Sent: Friday, May 25, 2018 8:56 AM
> To: Venkatesh Srinivas <venkateshs@google.com>
> Cc: vsrinivas@ops101.org; intel-wired-lan <intel-wired-lan@lists.osuosl.org>
> Subject: Re: [Intel-wired-lan] [PATCH] igb: Use dma_wmb() instead of wmb()
> before doorbell writes
> 
> On Thu, May 24, 2018 at 9:13 PM, Venkatesh Srinivas
> <venkateshs@google.com> wrote:
> > igb writes to doorbells to post transmit and receive descriptors;
> > after writing descriptors to memory but before writing to doorbells,
> > use dma_wmb() rather than wmb(). wmb() is more heavyweight than
> > necessary before doorbell writes.
> >
> > On x86, this avoids SFENCEs before doorbell writes in both the
> > tx and rx refill paths.
> >
> > Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Patch

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index cce7ada89255..8aea50867a5d 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -5674,7 +5674,7 @@  static int igb_tx_map(struct igb_ring *tx_ring,
 	 * We also need this memory barrier to make certain all of the
 	 * status bits have been updated before next_to_watch is written.
 	 */
-	wmb();
+	dma_wmb();
 
 	/* set next_to_watch value indicating a packet is present */
 	first->next_to_watch = tx_desc;
@@ -8093,7 +8093,7 @@  void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)
 		 * applicable for weak-ordered memory model archs,
 		 * such as IA-64).
 		 */
-		wmb();
+		dma_wmb();
 		writel(i, rx_ring->tail);
 	}
 }