[dpdk-dev] net/mlx5: fix Tx doorbell memory barrier

  Configuring UAR as IO-mapped makes maximum throughput decline by noticeable
amount. If UAR is configured as write-combining register, a write memory
barrier is needed on ringing a doorbell. rte_wmb() is mostly effective when
the size of a burst is comparatively small.

Fixes: 9f9bebae5530 ("net/mlx5: don't map doorbell register to write combining")
Cc: stable@dpdk.org
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Alexander Solganik <solganik@gmail.com>

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
Acked-by: Shahaf Shuler <shahafs@mellanox.com>
---
 drivers/net/mlx5/mlx5.c               | 2 --
 drivers/net/mlx5/mlx5_rxtx.c          | 8 ++++----
 drivers/net/mlx5/mlx5_rxtx.h          | 6 +++++-
 drivers/net/mlx5/mlx5_rxtx_vec_neon.h | 4 ++--
 drivers/net/mlx5/mlx5_rxtx_vec_sse.h  | 4 ++--
 5 files changed, 13 insertions(+), 11 deletions(-)
  

> Configuring UAR as IO-mapped makes maximum throughput decline by noticeable
> amount. If UAR is configured as write-combining register, a write memory
> barrier is needed on ringing a doorbell. rte_wmb() is mostly effective when
> the size of a burst is comparatively small.

Personally I don't think that the flag is really a good interface
choice. But also I'm not convinced that its dependent on the burst size.

What guarantees that even for larger bursts the mmio write was flushed?
it comes after a set of writes that were flushed prior to the db update
and its not guaranteed that the application will immediately have more
data to trigger this writes to flush.
  

On Sun, Oct 22, 2017 at 12:46:53PM +0300, Sagi Grimberg wrote:
> 
> > Configuring UAR as IO-mapped makes maximum throughput decline by noticeable
> > amount. If UAR is configured as write-combining register, a write memory
> > barrier is needed on ringing a doorbell. rte_wmb() is mostly effective when
> > the size of a burst is comparatively small.
> 
> Personally I don't think that the flag is really a good interface
> choice. But also I'm not convinced that its dependent on the burst size.
> 
> What guarantees that even for larger bursts the mmio write was flushed?
> it comes after a set of writes that were flushed prior to the db update
> and its not guaranteed that the application will immediately have more
> data to trigger this writes to flush.

Yes, I already knew the concern. I don't know you were aware but that can only
happen when burst size is exactly multiple of 32 in the vectorized Tx. If you
look at the mlx5_tx_burst_raw_vec(), every Tx bursts having more than 32 packets
will be calling txq_burst_v() more than one time. For example, if pkts_n is 45,
then it will firstly call txq_burst_v(32) and txq_burst_v(13) will follow with
setting barrier at the end. The only pitfall is when pkts_n is exactly multiple
of 32, e.g. 32, 64, 96 and so on. This shall not be likely when an app is
forwarding packets and the rate is low (if packet rate is high, we are good).
So, the only possible case of it is when an app generates traffic at
comparatively low rate in bursty way with burst size being multiple of 32. If a
user encounters such a rare case and latency is critical in their app, we will
recommend to set MLX5_SHUT_UP_BF=1 either by exporting in a shell or by
embedding it in their app's initialization. Or, they can use other
non-vectorized tx_burst() functions because the barrier is still enforced in
such functions like you firstly suggested.

It is always true that we can't make everyone satisfied. Some apps prefers
better performance to better latency. As vectorized Tx outperforms all the other
tx_burst() functions, I want to leave it as only one exceptional case. Actually,
we already received a complaint that 1-core performance of vPMD declined by 10%
(53Mpps -> 48Mpps) due to the patch (MLX5_SHUT_UP_BF=1). So, I wanted to give
users/apps more versatile options/knobs.

Before sending out this patch, I've done RFC2544 latency tests with Ixia and the
result was as good as before (actually same). That's why we think it is a good
compromise. 


Thanks for your comment,
Yongseok
  

On Sun, Oct 22, 2017 at 01:00:22AM -0700, Yongseok Koh wrote:
>[...]
> --- a/drivers/net/mlx5/mlx5_rxtx.h
> +++ b/drivers/net/mlx5/mlx5_rxtx.h
> @@ -584,9 +584,11 @@ mlx5_tx_mb2mr(struct mlx5_txq_data *txq, struct rte_mbuf *mb)
>   *   Pointer to TX queue structure.
>   * @param wqe
>   *   Pointer to the last WQE posted in the NIC.
> + * @param mb
> + *   Request for write memory barrier after BF update.

BF should be replaced by its real name i.e. BlueFlame.
If the mb is only related to the BlueFlame, why not naming it mbbf or
bfmf?

>   */
>  static __rte_always_inline void
> -mlx5_tx_dbrec(struct mlx5_txq_data *txq, volatile struct mlx5_wqe *wqe)
> +mlx5_tx_dbrec(struct mlx5_txq_data *txq, volatile struct mlx5_wqe *wqe, int mb)
>  {
>  	uint64_t *dst = (uint64_t *)((uintptr_t)txq->bf_reg);
>  	volatile uint64_t *src = ((volatile uint64_t *)wqe);
> @@ -596,6 +598,8 @@ mlx5_tx_dbrec(struct mlx5_txq_data *txq, volatile struct mlx5_wqe *wqe)
>  	/* Ensure ordering between DB record and BF copy. */
>  	rte_wmb();
>  	*dst = *src;
> +	if (mb)
> +		rte_wmb();
>  }

My last point, this modification seems necessary only for the MPS vector
case, why not adding a new function for this specific case instead?

At least I would suggest to create an enum with the possible values it
helps to read the source:

 enum mlx5_blueflame_mb {
 	MLX5_BLUEFLAME_MB_DIS, 
 	MLX5_BLUEFLAME_MB_EN, 
 }

you can also create wrappers for the function to avoid this extract
argument in the tx code:

 mlx5_tx_dbrec_bfmb(txq, wqe);
 mlx5_tx_dbrec_no_bfmb(txq, wqe);

Multiple ways instead of having 0 or 1 hard coded.

Thanks,
  

Yongseok, Sagi, my small contribution to this discussion,

On Sun, Oct 22, 2017 at 03:01:04PM -0700, Yongseok Koh wrote:
> On Sun, Oct 22, 2017 at 12:46:53PM +0300, Sagi Grimberg wrote:
> > 
> > > Configuring UAR as IO-mapped makes maximum throughput decline by noticeable
> > > amount. If UAR is configured as write-combining register, a write memory
> > > barrier is needed on ringing a doorbell. rte_wmb() is mostly effective when
> > > the size of a burst is comparatively small.
> > 
> > Personally I don't think that the flag is really a good interface
> > choice. But also I'm not convinced that its dependent on the burst size.
> > 
> > What guarantees that even for larger bursts the mmio write was flushed?
> > it comes after a set of writes that were flushed prior to the db update
> > and its not guaranteed that the application will immediately have more
> > data to trigger this writes to flush.
> 
> Yes, I already knew the concern. I don't know you were aware but that can only
> happen when burst size is exactly multiple of 32 in the vectorized Tx. If you
> look at the mlx5_tx_burst_raw_vec(), every Tx bursts having more than 32 packets
> will be calling txq_burst_v() more than one time. For example, if pkts_n is 45,
> then it will firstly call txq_burst_v(32) and txq_burst_v(13) will follow with
> setting barrier at the end. The only pitfall is when pkts_n is exactly multiple
> of 32, e.g. 32, 64, 96 and so on. This shall not be likely when an app is
> forwarding packets and the rate is low (if packet rate is high, we are good).
>
> So, the only possible case of it is when an app generates traffic at
> comparatively low rate in bursty way with burst size being multiple of 32.

A routing application will consume more than the 50 cycles the PMD needs
to process such burst.  It is not a rare case, it is the real one,
routing lookup table, parsing the packet to find the layers, modifying
them (decreasing the TTL, changing addresses and updating the checksum)
is not something fast.
The probability to have a full 32 packets burst entering in the PMD is
something "normal".

> If a user encounters such a rare case and latency is critical in their app, we will
> recommend to set MLX5_SHUT_UP_BF=1 either by exporting in a shell or by
> embedding it in their app's initialization. Or, they can use other
> non-vectorized tx_burst() functions because the barrier is still enforced in
> such functions like you firstly suggested.
> 
> It is always true that we can't make everyone satisfied. Some apps prefers
> better performance to better latency. As vectorized Tx outperforms all the other
> tx_burst() functions, I want to leave it as only one exceptional case. Actually,
> we already received a complaint that 1-core performance of vPMD declined by 10%
> (53Mpps -> 48Mpps) due to the patch (MLX5_SHUT_UP_BF=1). So, I wanted to give
> users/apps more versatile options/knobs.

DPDK is written for throughput, that the reason why Tx/Rx burst
functions are "burst" and written to consider it will process a large
amount of packets at each call and this to minimize the cost of all
memory barriers and doorbells.

If there are some application which also needs latency, it is maybe time
to introduce new API for that i.e. a dedicated to send/receive with a single
packet or a little more in an efficient way.
By the way, I am not sure such application handles such big sizes of
burst, Sagi you may have more informations, that you may want to share.

> Before sending out this patch, I've done RFC2544 latency tests with Ixia and the
> result was as good as before (actually same). That's why we think it is a good
> compromise. 

You cannot do that with testpmd, it does not match the real application
behavior as it receives a burst of packets and send them back without
touching them.  An application will at least process/route all received
packets to some other destination or port.  The send action will only be
triggered when the whole routing process is finished to maximize the
burst sizes.  According to the traffic, the latency will change.
From what I know, we don't have such kind of example/tool in DPDK.

> Thanks for your comment,
> Yongseok

Thanks,
  

On Mon, Oct 23, 2017 at 09:50:14AM +0200, Nélio Laranjeiro wrote:
> Yongseok, Sagi, my small contribution to this discussion,
> 
> On Sun, Oct 22, 2017 at 03:01:04PM -0700, Yongseok Koh wrote:
> > On Sun, Oct 22, 2017 at 12:46:53PM +0300, Sagi Grimberg wrote:
> > > 
> > > > Configuring UAR as IO-mapped makes maximum throughput decline by noticeable
> > > > amount. If UAR is configured as write-combining register, a write memory
> > > > barrier is needed on ringing a doorbell. rte_wmb() is mostly effective when
> > > > the size of a burst is comparatively small.
> > > 
> > > Personally I don't think that the flag is really a good interface
> > > choice. But also I'm not convinced that its dependent on the burst size.
> > > 
> > > What guarantees that even for larger bursts the mmio write was flushed?
> > > it comes after a set of writes that were flushed prior to the db update
> > > and its not guaranteed that the application will immediately have more
> > > data to trigger this writes to flush.
> > 
> > Yes, I already knew the concern. I don't know you were aware but that can only
> > happen when burst size is exactly multiple of 32 in the vectorized Tx. If you
> > look at the mlx5_tx_burst_raw_vec(), every Tx bursts having more than 32 packets
> > will be calling txq_burst_v() more than one time. For example, if pkts_n is 45,
> > then it will firstly call txq_burst_v(32) and txq_burst_v(13) will follow with
> > setting barrier at the end. The only pitfall is when pkts_n is exactly multiple
> > of 32, e.g. 32, 64, 96 and so on. This shall not be likely when an app is
> > forwarding packets and the rate is low (if packet rate is high, we are good).
> >
> > So, the only possible case of it is when an app generates traffic at
> > comparatively low rate in bursty way with burst size being multiple of 32.
> 
> A routing application will consume more than the 50 cycles the PMD needs
> to process such burst.  It is not a rare case, it is the real one,
> routing lookup table, parsing the packet to find the layers, modifying
> them (decreasing the TTL, changing addresses and updating the checksum)
> is not something fast.
> The probability to have a full 32 packets burst entering in the PMD is
> something "normal".

Right. There could be more common cases of skipping the barrier. And that's a
good example. But my point here is to give more options to users, not to defend
all the possible singular cases. If an app is processing packets in bursty way,
it is already compromise between latency and throughput. Like you mentioned
below, that's one of design goals of DPDK. If latency is so critical, it would
be better to use interrupt driven processing in kernel context.

Back to original issue, MLX5 PMD had unreasonably high max latency, especially
with low rate of traffic while min/avg was okay and this could be considered as
a critical bug. This sort of patches are to resolve that issue, not to improve
its overall latency results.

[...]
> > Before sending out this patch, I've done RFC2544 latency tests with Ixia and the
> > result was as good as before (actually same). That's why we think it is a good
> > compromise. 
> 
> You cannot do that with testpmd, it does not match the real application
> behavior as it receives a burst of packets and send them back without
> touching them.  An application will at least process/route all received
> packets to some other destination or port.  The send action will only be
> triggered when the whole routing process is finished to maximize the
> burst sizes.  According to the traffic, the latency will change.
> From what I know, we don't have such kind of example/tool in DPDK.

I wasn't saying that the test with testpmd represented real time use-cases but I
just wanted to verify this patch is effective to resolve the original issue.
Again, this isn't a patch for latency enhancement but to resolve the issue.

And I think I should also change documentation to address the option
(MLX5_SHUT_UP_BF=1) for v2, unless there's objection.


Thanks,
Yongseok
  

On Mon, Oct 23, 2017 at 10:24:09AM -0700, Yongseok Koh wrote:
> On Mon, Oct 23, 2017 at 09:50:14AM +0200, Nélio Laranjeiro wrote:
> > Yongseok, Sagi, my small contribution to this discussion,
> > 
> > On Sun, Oct 22, 2017 at 03:01:04PM -0700, Yongseok Koh wrote:
> > > On Sun, Oct 22, 2017 at 12:46:53PM +0300, Sagi Grimberg wrote:
> > > > 
> > > > > Configuring UAR as IO-mapped makes maximum throughput decline by noticeable
> > > > > amount. If UAR is configured as write-combining register, a write memory
> > > > > barrier is needed on ringing a doorbell. rte_wmb() is mostly effective when
> > > > > the size of a burst is comparatively small.
> > > > 
> > > > Personally I don't think that the flag is really a good interface
> > > > choice. But also I'm not convinced that its dependent on the burst size.
> > > > 
> > > > What guarantees that even for larger bursts the mmio write was flushed?
> > > > it comes after a set of writes that were flushed prior to the db update
> > > > and its not guaranteed that the application will immediately have more
> > > > data to trigger this writes to flush.
> > > 
> > > Yes, I already knew the concern. I don't know you were aware but that can only
> > > happen when burst size is exactly multiple of 32 in the vectorized Tx. If you
> > > look at the mlx5_tx_burst_raw_vec(), every Tx bursts having more than 32 packets
> > > will be calling txq_burst_v() more than one time. For example, if pkts_n is 45,
> > > then it will firstly call txq_burst_v(32) and txq_burst_v(13) will follow with
> > > setting barrier at the end. The only pitfall is when pkts_n is exactly multiple
> > > of 32, e.g. 32, 64, 96 and so on. This shall not be likely when an app is
> > > forwarding packets and the rate is low (if packet rate is high, we are good).
> > >
> > > So, the only possible case of it is when an app generates traffic at
> > > comparatively low rate in bursty way with burst size being multiple of 32.
> > 
> > A routing application will consume more than the 50 cycles the PMD needs
> > to process such burst.  It is not a rare case, it is the real one,
> > routing lookup table, parsing the packet to find the layers, modifying
> > them (decreasing the TTL, changing addresses and updating the checksum)
> > is not something fast.
> > The probability to have a full 32 packets burst entering in the PMD is
> > something "normal".
> 
> Right. There could be more common cases of skipping the barrier. And that's a
> good example. But my point here is to give more options to users, not to defend
> all the possible singular cases. If an app is processing packets in bursty way,
> it is already compromise between latency and throughput. Like you mentioned
> below, that's one of design goals of DPDK. If latency is so critical, it would
> be better to use interrupt driven processing in kernel context.
> 
> Back to original issue, MLX5 PMD had unreasonably high max latency, especially
> with low rate of traffic while min/avg was okay and this could be considered as
> a critical bug. This sort of patches are to resolve that issue, not to improve
> its overall latency results.
> 
> [...]
> > > Before sending out this patch, I've done RFC2544 latency tests with Ixia and the
> > > result was as good as before (actually same). That's why we think it is a good
> > > compromise. 
> > 
> > You cannot do that with testpmd, it does not match the real application
> > behavior as it receives a burst of packets and send them back without
> > touching them.  An application will at least process/route all received
> > packets to some other destination or port.  The send action will only be
> > triggered when the whole routing process is finished to maximize the
> > burst sizes.  According to the traffic, the latency will change.
> > From what I know, we don't have such kind of example/tool in DPDK.
> 
> I wasn't saying that the test with testpmd represented real time use-cases

I certain of that, but it is mostly because I know how you work, others
DPDK contributors entering in this thread, may understand this ;).

> but I just wanted to verify this patch is effective to resolve the
> original issue.
> Again, this isn't a patch for latency enhancement but to resolve the issue.
>
> And I think I should also change documentation to address the option
> (MLX5_SHUT_UP_BF=1) for v2, unless there's objection.

If letting it has a huge impact on the throughput performance and it is
the reason why the PMD won't set it in the future, it should be
documented to let users embed it in their environment. You can also
provide some information about the impact of activating such behavior.

Thanks,