i40e/i40evf: Eliminate duplicate barriers on weakly-ordered archs

Code includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

Code includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/qedr/verbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Quoted 7 lines · Sinan Kaya

Code includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the
register write.

Quoted 6 lines

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

 drivers/infiniband/hw/qedr/verbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Sure matches my understanding of writel_relaxed

This is part of a series, should we take just this patch through the rdma tree? If not:

Acked-by Jason Gunthorpe <[email protected]>

Thanks,
Jason

Code includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

Acked-by Jason Gunthorpe <[email protected]>

 drivers/infiniband/hw/qedr/verbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Code includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/net/ethernet/intel/igbvf/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/intel/igbvf/netdev.c +0 -0

index 4214c15..fe3441b 100644

Code includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ethernet/intel/igb/igb_main.c +0 -0

index b88fae7..ba8ccb5 100644

Code includes wmb() followed by writel(). writel() already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/net/ethernet/intel/e1000/e1000_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/ethernet/intel/e1000/e1000_main.c +1 -1

index 3dd4aeb..e0e583a 100644
@@ -4573,7 +4573,7 @@ e1000_alloc_jumbo_rx_buffers(struct e1000_adapter *adapter,
                 * such as IA-64).
                 */
                wmb();
-               writel(i, adapter->hw.hw_addr + rx_ring->rdt);
+               writel_relaxed(i, adapter->hw.hw_addr + rx_ring->rdt);
        }
 }
 

Quoted 2 lines

}

So you missed the writel in e1000_xmit_frame. You should probably get that one too while you are doing these updates. The wmb() is in e1000_tx_queue().

Quoted 6 lines · Alexander Duyck

Quoted 2 lines

}

So you missed the writel in e1000_xmit_frame. You should probably get that one too while you are doing these updates. The wmb() is in e1000_tx_queue().

I brought wmb() outside along with the next descriptor assignment to be similar to the rest of the other code.

if wmb() and writel() are not visible in the same function, let's not touch the code.

Quoted 11 lines · Sinan Kaya

Quoted 6 lines · Alexander Duyck

Quoted 2 lines

}

So you missed the writel in e1000_xmit_frame. You should probably get that one too while you are doing these updates. The wmb() is in e1000_tx_queue().

I brought wmb() outside along with the next descriptor assignment to be similar to the rest of the other code.

Quoted 2 lines

if wmb() and writel() are not visible in the same function, let's not touch the code.

Maybe for e1000 we should just skip the driver entirely. Odds are you aren't going to have any e1000 parts running on ARM anyway since most of them are legacy PCI or PCI-X parts that were made over 10 years ago. Most of your efforts would probably be best spent on igb, igbvf, ixgbe, ixgbevf, i40e, i40evf, and fm10k.

drivers/net/ethernet/intel/ixgbevf/ixgbevf.h static inline u16 ixgbevf_desc_unused(struct ixgbevf_ring *ring) · line 253

Quoted 10 lines · Sinan Kaya

+/* Assumes caller has executed a write barrier to order memory and device
+ * requests.
+ */
static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32
value)
{

drivers/net/ethernet/intel/ixgbevf/ixgbevf.h +1 -1

-	writel(value, ring->tail);
+	writel_relaxed(value, ring->tail);
  }

Why not put the wmb() in this function, or just get rid of the wmb() in the rest of the file and keep this as writel? That way, you can avoid the comment and the risk that comes with it.

Quoted 15 lines · Timur Tabi

Quoted 11 lines · Sinan Kaya

+/* Assumes caller has executed a write barrier to order memory and device
+ * requests.
+ */
static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32
value)
{

drivers/net/ethernet/intel/ixgbevf/ixgbevf.h +1 -1

-       writel(value, ring->tail);
+       writel_relaxed(value, ring->tail);
  }

Quoted 3 lines

Why not put the wmb() in this function, or just get rid of the wmb() in the rest of the file and keep this as writel? That way, you can avoid the comment and the risk that comes with it.

Sure, both solutions will work. I want to see what the maintainer prefers. I can repost accordingly.

Quoted 3 lines · On Wed, Mar 14, 2018 at 5:13 AM, <[email protected]> wrote:

Quoted 2 lines · Timur Tabi

On 3/13/18 10:20 PM, Sinan Kaya wrote:

Actually I would argue this whole patch set is pointless. For starters why is it we are only updating the Intel Ethernet drivers here?

This
seems like something that is going to impact the whole kernel tree since many of us have been writing drivers for some time assuming x86 style behavior.

It doesn't make sense to be optimizing the drivers for one subset of architectures. The scope of the work needed to update the drivers for this would be ridiculous. Also I don't see how this could be expected to work on any other architecture when we pretty much need to have a wmb() before calling the writel on x86 to deal with accesses between coherent and non-coherent memory. It seems to me more like somebody added what they considered to be an optimization somewhere that is a workaround for a poorly written driver. Either that or the barrier is serving a different purpose then the one we were using.

It would make more sense to put in the effort making writel and writel_relaxed consistent between architectures before we go through and start modifying driver code to support different architectures.

Hi Alexander,

Quoted 3 lines · Alexander Duyck

Quoted 2 lines · On Wed, Mar 14, 2018 at 5:13 AM, <[email protected]> wrote:

Quoted 1 line · Timur Tabi

Quoted 4 lines

Quoted 1 line

Quoted 1 line · Sinan Kaya

Actually I would argue this whole patch set is pointless. For starters why is it we are only updating the Intel Ethernet drivers here?

I did a regex search for wmb() followed by writel() in each drivers directory. I scrubbed the ones I care about and posted this series. Note also that I have one Infiniband patch in the series.

I considered "ease of change", "popular usage" and "performance critical path" as the determining criteria for my filtering.

Quoted 4 lines

This
seems like something that is going to impact the whole kernel tree since many of us have been writing drivers for some time assuming x86 style behavior.

That's true. We used relaxed API heavily on ARM for a long time but it did not exist on other architectures. For this reason, relaxed architectures have been paying double penalty in order to use the common drivers.

Now that relaxed API is present on all architectures, we can go and scrub all drivers to see what needs to change and what can remain.

Quoted 10 lines

It doesn't make sense to be optimizing the drivers for one subset of architectures. The scope of the work needed to update the drivers for this would be ridiculous. Also I don't see how this could be expected to work on any other architecture when we pretty much need to have a wmb() before calling the writel on x86 to deal with accesses between coherent and non-coherent memory. It seems to me more like somebody added what they considered to be an optimization somewhere that is a workaround for a poorly written driver. Either that or the barrier is serving a different purpose then the one we were using.

Is there a semantic problem with the definition of wmb() vs. writel() vs. writel_relaxed()? I thought everything is well described in barriers document about what to expect from these APIs.

AFAIK, writel() is equal to writel_relaxed() on x86 architecture. It doesn't really change anything for x86 but it saves barriers on other architectures.

Quoted 5 lines

It would make more sense to put in the effort making writel and writel_relaxed consistent between architectures before we go through and start modifying driver code to support different architectures.

Is there an arch problem that I'm not seeing?

Sinan

Quoted 11 lines · Sinan Kaya

Hi Alexander,

Quoted 7 lines · Alexander Duyck

Quoted 3 lines · On Wed, Mar 14, 2018 at 5:13 AM, <[email protected]> wrote:

Quoted 2 lines · Timur Tabi

On 3/13/18 10:20 PM, Sinan Kaya wrote:

Actually I would argue this whole patch set is pointless. For starters why is it we are only updating the Intel Ethernet drivers here?

Quoted 3 lines

I did a regex search for wmb() followed by writel() in each drivers directory. I scrubbed the ones I care about and posted this series. Note also that I have one Infiniband patch in the series.

I didn't see it as it I was only looking at the patches that had ended up in intel-wired-lan. Also was there a cover page, I couldn't seem to find that on LKML.

Quoted 3 lines

I considered "ease of change", "popular usage" and "performance critical path" as the determining criteria for my filtering.

It might be advisable to break things up to subsystem or family. So for example if you are going to update the Intel Ethernet drivers I would focus on that and maybe spin the infiniband patch of into a separate set that can be applied to a separate tree. This is something I would consider more of a driver optimization than a fix. In our case it makes it easier for us to maintain the patches to the Intel drivers if you could submit just those to Jeff and Intel-wired-lan so that we can take care of test and review as well as figure out what other drivers will would still need to update in order to handle all the cases involved in this.

Quoted 10 lines

Quoted 4 lines

This
seems like something that is going to impact the whole kernel tree since many of us have been writing drivers for some time assuming x86 style behavior.

That's true. We used relaxed API heavily on ARM for a long time but it did not exist on other architectures. For this reason, relaxed architectures have been paying double penalty in order to use the common drivers.

Quoted 2 lines

Now that relaxed API is present on all architectures, we can go and scrub all drivers to see what needs to change and what can remain.

My only real objection is that you are going to be having to scrub pretty much ALL the drivers. It seems a little like trying to fix a bad tire on your car by paving the road to match the shape of the tire.

Quoted 16 lines

Quoted 11 lines

It doesn't make sense to be optimizing the drivers for one subset of architectures. The scope of the work needed to update the drivers for this would be ridiculous. Also I don't see how this could be expected to work on any other architecture when we pretty much need to have a wmb() before calling the writel on x86 to deal with accesses between coherent and non-coherent memory. It seems to me more like somebody added what they considered to be an optimization somewhere that is a workaround for a poorly written driver. Either that or the barrier is serving a different purpose then the one we were using.

Is there a semantic problem with the definition of wmb() vs. writel() vs. writel_relaxed()? I thought everything is well described in barriers document about what to expect from these APIs.

Quoted 3 lines

AFAIK, writel() is equal to writel_relaxed() on x86 architecture. It doesn't really change anything for x86 but it saves barriers on other architectures.

Yeah. I had to go through and do some review since my concerns have been PowerPC, IA64, and x86 historically. From what I can tell all those architectures are setup the same way so that shouldn't be an issue.

Quoted 9 lines

Quoted 6 lines

It would make more sense to put in the effort making writel and writel_relaxed consistent between architectures before we go through and start modifying driver code to support different architectures.

Is there an arch problem that I'm not seeing?

Quoted 1 line

Sinan

It isn't really an arch problem I have as a logistical one. It just seems like this is really backwards in terms of how this has been handled. For the x86 we have historically had to deal with the barriers for this kind of stuff ourselves, now for ARM and a couple other architectures they seem to have incorporated the barriers into writel and are expecting everyone to move over to writel_relaxed.

It
seems like instead of going that route they should have probably just looked at pushing the ARM drivers to something like a "writel_strict" and adopted the behavior of the other architectures for writel.

I'll go back through and review. It looks like a number of items were missed.

Quoted 12 lines · Alexander Duyck

Quoted 11 lines · Sinan Kaya

Hi Alexander,

Quoted 7 lines · Alexander Duyck

Quoted 3 lines · On Wed, Mar 14, 2018 at 5:13 AM, <[email protected]> wrote:

Quoted 2 lines · Timur Tabi

On 3/13/18 10:20 PM, Sinan Kaya wrote:

Actually I would argue this whole patch set is pointless. For starters why is it we are only updating the Intel Ethernet drivers here?

Quoted 7 lines

Quoted 3 lines

I did a regex search for wmb() followed by writel() in each drivers directory. I scrubbed the ones I care about and posted this series. Note also that I have one Infiniband patch in the series.

I didn't see it as it I was only looking at the patches that had ended up in intel-wired-lan. Also was there a cover page, I couldn't seem to find that on LKML.

Yeah, I didn't have a cover page. These patches were sitting on my branch for a while. I wanted to get them out without putting too much effort into it. I'll add it on the next version.

Quoted 25 lines

Quoted 3 lines

I considered "ease of change", "popular usage" and "performance critical path" as the determining criteria for my filtering.

It might be advisable to break things up to subsystem or family. So for example if you are going to update the Intel Ethernet drivers I would focus on that and maybe spin the infiniband patch of into a separate set that can be applied to a separate tree. This is something I would consider more of a driver optimization than a fix. In our case it makes it easier for us to maintain the patches to the Intel drivers if you could submit just those to Jeff and Intel-wired-lan so that we can take care of test and review as well as figure out what other drivers will would still need to update in order to handle all the cases involved in this.

Quoted 10 lines

Quoted 4 lines

This
seems like something that is going to impact the whole kernel tree since many of us have been writing drivers for some time assuming x86 style behavior.

That's true. We used relaxed API heavily on ARM for a long time but it did not exist on other architectures. For this reason, relaxed architectures have been paying double penalty in order to use the common drivers.

Quoted 7 lines

Quoted 2 lines

Now that relaxed API is present on all architectures, we can go and scrub all drivers to see what needs to change and what can remain.

My only real objection is that you are going to be having to scrub pretty much ALL the drivers. It seems a little like trying to fix a bad tire on your car by paving the road to match the shape of the tire.

Or we start with mostly used ones and hope to increase the coverage over time. It will take a while to cover all drivers.

We could do several iterations like you are suggesting for each subsystem.

Quoted 16 lines

Quoted 16 lines

Quoted 11 lines

It doesn't make sense to be optimizing the drivers for one subset of architectures. The scope of the work needed to update the drivers for this would be ridiculous. Also I don't see how this could be expected to work on any other architecture when we pretty much need to have a wmb() before calling the writel on x86 to deal with accesses between coherent and non-coherent memory. It seems to me more like somebody added what they considered to be an optimization somewhere that is a workaround for a poorly written driver. Either that or the barrier is serving a different purpose then the one we were using.

Is there a semantic problem with the definition of wmb() vs. writel() vs. writel_relaxed()? I thought everything is well described in barriers document about what to expect from these APIs.

Quoted 8 lines

Quoted 3 lines

AFAIK, writel() is equal to writel_relaxed() on x86 architecture. It doesn't really change anything for x86 but it saves barriers on other architectures.

Yeah. I had to go through and do some review since my concerns have been PowerPC, IA64, and x86 historically. From what I can tell all those architectures are setup the same way so that shouldn't be an issue.

OK, glad that we are in common understanding.

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

It would make more sense to put in the effort making writel and writel_relaxed consistent between architectures before we go through and start modifying driver code to support different architectures.

Is there an arch problem that I'm not seeing?

Quoted 8 lines

Quoted 1 line

Sinan

It isn't really an arch problem I have as a logistical one. It just seems like this is really backwards in terms of how this has been handled. For the x86 we have historically had to deal with the barriers for this kind of stuff ourselves, now for ARM and a couple other architectures they seem to have incorporated the barriers into writel and are expecting everyone to move over to writel_relaxed.

You want to move to writel_relaxed() only if you know that your register accesses won't have any side effects.

If you require some memory update to be observable to the HW before doing a register write, right thing is to do

wmb() + writel_relaxed()

wmb() + writel() is clearly the wrong choice and that's what the goal of this change.

If we know that all writel() adaptations in all architectures guarantee observability, another option is to get rid of wmb() and just keep writel().

I'm not so convinced about this and hoping that someone will correct me.

wmb() on x86 seems to have an sfence but writel() seems to have a compiler barrier in it. So, the type of barrier wmb() is using is different.

we can't say

(wmb()+ writel_relaxed()) == writel()

for all architectures, but maybe I'm wrong.

We really don't want to convert all writel() to writel_relaxed() blindly without giving much thought into it.

This was also another reason why I limited the changes to wmb() + writel() combinations only.

If there is wmb() + code + writel() and if we convert this to wmb() + code + writel_relaxed(), code will not be observed by the HW and this might break the driver.

Quoted 7 lines

It
seems like instead of going that route they should have probably just looked at pushing the ARM drivers to something like a "writel_strict" and adopted the behavior of the other architectures for writel.

I'll go back through and review. It looks like a number of items were missed.

OK, I'll take a look at each one. Saw the code concern on the e1000e suggestion. I wanted to raise it here first before inspecting the rest.

We tend to do something like:
update tx_buffer_info
update tx_desc
wmb()
point first tx_buffer_info next_to_watch value at last tx_desc update next_to_use
notify device via writel

We do it this way because we have to synchronize between the Tx cleanup path and the hardware so we basically lump the two barriers together. instead of invoking both a smp_wmb and a wmb. Now that I look at the pseudocode though I wonder if we shouldn't move the next_to_use update before the wmb, but that might be material for another patch. Anyway, in the Tx cleanup path we should have an smp_rmb() after we read the next_to_watch values so that we avoid reading any of the other fields in the buffer_info if either the field is NULL or the descriptor pointed to has not been written back.

Quoted 8 lines · Alexander Duyck

We tend to do something like:
update tx_buffer_info
update tx_desc
wmb()
point first tx_buffer_info next_to_watch value at last tx_desc update next_to_use
notify device via writel

Quoted 9 lines

We do it this way because we have to synchronize between the Tx cleanup path and the hardware so we basically lump the two barriers together. instead of invoking both a smp_wmb and a wmb. Now that I look at the pseudocode though I wonder if we shouldn't move the next_to_use update before the wmb, but that might be material for another patch. Anyway, in the Tx cleanup path we should have an smp_rmb() after we read the next_to_watch values so that we avoid reading any of the other fields in the buffer_info if either the field is NULL or the descriptor pointed to has not been written back.

How do you feel about keeping wmb() very close to writel_relaxed() like this?

update tx_buffer_info
update tx_desc
point first tx_buffer_info next_to_watch value at last tx_desc update next_to_use
wmb()
notify device via writel_relaxed()

I'm afraid that if the order of wmb() and writel() is not very obvious or hidden in multiple functions, somebody can introduce a very nasty bug in the future.

We also have to think about code maintenance.

Quoted 28 lines · Sinan Kaya

Quoted 8 lines · Alexander Duyck

We tend to do something like:
update tx_buffer_info
update tx_desc
wmb()
point first tx_buffer_info next_to_watch value at last tx_desc update next_to_use
notify device via writel

Quoted 9 lines

We do it this way because we have to synchronize between the Tx cleanup path and the hardware so we basically lump the two barriers together. instead of invoking both a smp_wmb and a wmb. Now that I look at the pseudocode though I wonder if we shouldn't move the next_to_use update before the wmb, but that might be material for another patch. Anyway, in the Tx cleanup path we should have an smp_rmb() after we read the next_to_watch values so that we avoid reading any of the other fields in the buffer_info if either the field is NULL or the descriptor pointed to has not been written back.

How do you feel about keeping wmb() very close to writel_relaxed() like this?

update tx_buffer_info
update tx_desc
point first tx_buffer_info next_to_watch value at last tx_desc update next_to_use
wmb()
notify device via writel_relaxed()

Quoted 6 lines

I'm afraid that if the order of wmb() and writel() is not very obvious or hidden in multiple functions, somebody can introduce a very nasty bug in the future.

We also have to think about code maintenance.

Now that I read your email again, I think this is the reason if I understood you correctly.

"instead of invoking both a smp_wmb and a wmb"

You'd need something like

update tx_buffer_info
update tx_desc
smp_wmb()
point first tx_buffer_info next_to_watch value at last tx_desc update next_to_use
wmb()
notify device via writel_relaxed()

Let me work on your comments.

Code includes wmb() followed by writel() in multiple places. writel() already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing the register write.

Since code already has an explicit barrier call, changing writel() to writel_relaxed().

Signed-off-by Sinan Kaya <[email protected]>

---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h      | 5 ++++-
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 7 +++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

drivers/net/ethernet/intel/ixgbevf/ixgbevf.h +4 -1

index f695242..64d0e0b 100644
@@ -244,9 +244,12 @@ static inline u16 ixgbevf_desc_unused(struct ixgbevf_ring *ring)
        return ((ntc > ntu) ? 0 : ring->count) + ntc - ntu - 1;
 }
 
+/* Assumes caller has executed a write barrier to order memory and device
+ * requests.
+ */
 static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32 value)
 {
-       writel(value, ring->tail);
+       writel_relaxed(value, ring->tail);
 }
 
 #define IXGBEVF_RX_DESC(R, i)  \

drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +0 -0

index 9b3d43d..0ba7f59 100644

drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c +1 -1

+
        /* notify HW of packet */
        ixgbevf_write_tail(tx_ring, i);
 
--

2.7.4