| Message ID | 20200903210022.22774-5-saeedm@nvidia.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Driver Reviewers |
| Headers | show |
| Series | [net-next,01/10] net/mlx5e: Refactor inline header size calculation in the TX path | expand |
On Thu, Sep 3, 2020 at 11:01 PM Saeed Mahameed <saeedm@nvidia.com> wrote: > > From: Maxim Mikityanskiy <maximmi@mellanox.com> > > A constant for the number of DS in an empty WQE (i.e. a WQE without data > segments) is needed in multiple places (normal TX data path, MPWQE in > XDP), but currently we have a constant for XDP and an inline formula in > normal TX. This patch introduces a common constant. > > Additionally, mlx5e_xdp_mpwqe_session_start is converted to use struct > assignment, because the code nearby is touched. > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > --- > .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 2 ++ > .../net/ethernet/mellanox/mlx5/core/en/xdp.c | 13 +++++++----- > .../net/ethernet/mellanox/mlx5/core/en/xdp.h | 21 +++++++------------ > .../net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +- > 4 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h > index d4ee22789ab0..155b89998891 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h > @@ -7,6 +7,8 @@ > #include "en.h" > #include <linux/indirect_call_wrapper.h> > > +#define MLX5E_TX_WQE_EMPTY_DS_COUNT (sizeof(struct mlx5e_tx_wqe) / MLX5_SEND_WQE_DS) > + Out of curiosity, what is the logic for dividing this struct by 16? struct mlx5e_tx_wqe { struct mlx5_wqe_ctrl_seg ctrl; struct mlx5_wqe_eth_seg eth; struct mlx5_wqe_data_seg data[0]; }; > #define INL_HDR_START_SZ (sizeof(((struct mlx5_wqe_eth_seg *)NULL)->inline_hdr.start)) > > enum mlx5e_icosq_wqe_type { > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > index 7fccd2ea7dc9..81cd9a04bcb0 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c > @@ -196,16 +196,19 @@ static void mlx5e_xdp_mpwqe_session_start(struct mlx5e_xdpsq *sq) > { > struct mlx5e_xdp_mpwqe *session = &sq->mpwqe; > struct mlx5e_xdpsq_stats *stats = sq->stats; > + struct mlx5e_tx_wqe *wqe; > u16 pi; > > pi = mlx5e_xdpsq_get_next_pi(sq, MLX5E_XDP_MPW_MAX_WQEBBS); > - session->wqe = MLX5E_TX_FETCH_WQE(sq, pi); > - > + wqe = MLX5E_TX_FETCH_WQE(sq, pi); > net_prefetchw(session->wqe->data); Is this prefetch still valid? And is the temporary variable wqe still needed at all? > - session->ds_count = MLX5E_XDP_TX_EMPTY_DS_COUNT; > - session->pkt_count = 0; > > - mlx5e_xdp_update_inline_state(sq); > + *session = (struct mlx5e_xdp_mpwqe) { > + .wqe = wqe, > + .ds_count = MLX5E_TX_WQE_EMPTY_DS_COUNT, > + .pkt_count = 0, > + .inline_on = mlx5e_xdp_get_inline_state(sq, session->inline_on), > + }; > > stats->mpwqe++; > }
On 2020-09-04 18:05, Willem de Bruijn wrote: > On Thu, Sep 3, 2020 at 11:01 PM Saeed Mahameed <saeedm@nvidia.com> wrote: >> >> From: Maxim Mikityanskiy <maximmi@mellanox.com> >> >> A constant for the number of DS in an empty WQE (i.e. a WQE without data >> segments) is needed in multiple places (normal TX data path, MPWQE in >> XDP), but currently we have a constant for XDP and an inline formula in >> normal TX. This patch introduces a common constant. >> >> Additionally, mlx5e_xdp_mpwqe_session_start is converted to use struct >> assignment, because the code nearby is touched. >> >> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> >> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> >> --- >> .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 2 ++ >> .../net/ethernet/mellanox/mlx5/core/en/xdp.c | 13 +++++++----- >> .../net/ethernet/mellanox/mlx5/core/en/xdp.h | 21 +++++++------------ >> .../net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +- >> 4 files changed, 19 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >> index d4ee22789ab0..155b89998891 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h >> @@ -7,6 +7,8 @@ >> #include "en.h" >> #include <linux/indirect_call_wrapper.h> >> >> +#define MLX5E_TX_WQE_EMPTY_DS_COUNT (sizeof(struct mlx5e_tx_wqe) / MLX5_SEND_WQE_DS) >> + > > Out of curiosity, what is the logic for dividing this struct by 16? The hardware needs the size of a WQE in DS units (16 bytes). An empty WQE takes 2 DS (for the ctrl and eth segments), and this macro is this initial size of an empty WQE (2 DS). As data segments are added to the WQE, it grows, and its size in DS also grows. > struct mlx5e_tx_wqe { > struct mlx5_wqe_ctrl_seg ctrl; > struct mlx5_wqe_eth_seg eth; > struct mlx5_wqe_data_seg data[0]; > }; > >> #define INL_HDR_START_SZ (sizeof(((struct mlx5_wqe_eth_seg *)NULL)->inline_hdr.start)) >> >> enum mlx5e_icosq_wqe_type { >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c >> index 7fccd2ea7dc9..81cd9a04bcb0 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c >> @@ -196,16 +196,19 @@ static void mlx5e_xdp_mpwqe_session_start(struct mlx5e_xdpsq *sq) >> { >> struct mlx5e_xdp_mpwqe *session = &sq->mpwqe; >> struct mlx5e_xdpsq_stats *stats = sq->stats; >> + struct mlx5e_tx_wqe *wqe; >> u16 pi; >> >> pi = mlx5e_xdpsq_get_next_pi(sq, MLX5E_XDP_MPW_MAX_WQEBBS); >> - session->wqe = MLX5E_TX_FETCH_WQE(sq, pi); >> - >> + wqe = MLX5E_TX_FETCH_WQE(sq, pi); >> net_prefetchw(session->wqe->data); > > Is this prefetch still valid? It should be: net_prefetchw(wqe->data); Probably a bad rebase. > And is the temporary variable wqe still > needed at all? We want to prefetch as early as possible (before filling *session). > >> - session->ds_count = MLX5E_XDP_TX_EMPTY_DS_COUNT; >> - session->pkt_count = 0; >> >> - mlx5e_xdp_update_inline_state(sq); >> + *session = (struct mlx5e_xdp_mpwqe) { >> + .wqe = wqe, >> + .ds_count = MLX5E_TX_WQE_EMPTY_DS_COUNT, >> + .pkt_count = 0, >> + .inline_on = mlx5e_xdp_get_inline_state(sq, session->inline_on), >> + }; >> >> stats->mpwqe++; >> }
On Tue, Sep 8, 2020 at 10:59 AM Maxim Mikityanskiy <maximmi@nvidia.com> wrote: > > On 2020-09-04 18:05, Willem de Bruijn wrote: > > On Thu, Sep 3, 2020 at 11:01 PM Saeed Mahameed <saeedm@nvidia.com> wrote: > >> > >> From: Maxim Mikityanskiy <maximmi@mellanox.com> > >> > >> A constant for the number of DS in an empty WQE (i.e. a WQE without data > >> segments) is needed in multiple places (normal TX data path, MPWQE in > >> XDP), but currently we have a constant for XDP and an inline formula in > >> normal TX. This patch introduces a common constant. > >> > >> Additionally, mlx5e_xdp_mpwqe_session_start is converted to use struct > >> assignment, because the code nearby is touched. > >> > >> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > >> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > >> --- > >> .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 2 ++ > >> .../net/ethernet/mellanox/mlx5/core/en/xdp.c | 13 +++++++----- > >> .../net/ethernet/mellanox/mlx5/core/en/xdp.h | 21 +++++++------------ > >> .../net/ethernet/mellanox/mlx5/core/en_tx.c | 2 +- > >> 4 files changed, 19 insertions(+), 19 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h > >> index d4ee22789ab0..155b89998891 100644 > >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h > >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h > >> @@ -7,6 +7,8 @@ > >> #include "en.h" > >> #include <linux/indirect_call_wrapper.h> > >> > >> +#define MLX5E_TX_WQE_EMPTY_DS_COUNT (sizeof(struct mlx5e_tx_wqe) / MLX5_SEND_WQE_DS) > >> + > > > > Out of curiosity, what is the logic for dividing this struct by 16? > > The hardware needs the size of a WQE in DS units (16 bytes). An empty > WQE takes 2 DS (for the ctrl and eth segments), and this macro is this > initial size of an empty WQE (2 DS). As data segments are added to the > WQE, it grows, and its size in DS also grows. > > > struct mlx5e_tx_wqe { > > struct mlx5_wqe_ctrl_seg ctrl; > > struct mlx5_wqe_eth_seg eth; > > struct mlx5_wqe_data_seg data[0]; > > }; Thanks. It was not obvious to me that the first two are the size as data_segs. But that actually is pretty logical. Ack.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h index d4ee22789ab0..155b89998891 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h @@ -7,6 +7,8 @@ #include "en.h" #include <linux/indirect_call_wrapper.h> +#define MLX5E_TX_WQE_EMPTY_DS_COUNT (sizeof(struct mlx5e_tx_wqe) / MLX5_SEND_WQE_DS) + #define INL_HDR_START_SZ (sizeof(((struct mlx5_wqe_eth_seg *)NULL)->inline_hdr.start)) enum mlx5e_icosq_wqe_type { diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c index 7fccd2ea7dc9..81cd9a04bcb0 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c @@ -196,16 +196,19 @@ static void mlx5e_xdp_mpwqe_session_start(struct mlx5e_xdpsq *sq) { struct mlx5e_xdp_mpwqe *session = &sq->mpwqe; struct mlx5e_xdpsq_stats *stats = sq->stats; + struct mlx5e_tx_wqe *wqe; u16 pi; pi = mlx5e_xdpsq_get_next_pi(sq, MLX5E_XDP_MPW_MAX_WQEBBS); - session->wqe = MLX5E_TX_FETCH_WQE(sq, pi); - + wqe = MLX5E_TX_FETCH_WQE(sq, pi); net_prefetchw(session->wqe->data); - session->ds_count = MLX5E_XDP_TX_EMPTY_DS_COUNT; - session->pkt_count = 0; - mlx5e_xdp_update_inline_state(sq); + *session = (struct mlx5e_xdp_mpwqe) { + .wqe = wqe, + .ds_count = MLX5E_TX_WQE_EMPTY_DS_COUNT, + .pkt_count = 0, + .inline_on = mlx5e_xdp_get_inline_state(sq, session->inline_on), + }; stats->mpwqe++; } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h index 615bf04f4a54..96d6b1553bab 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.h @@ -38,9 +38,7 @@ #include "en/txrx.h" #define MLX5E_XDP_MIN_INLINE (ETH_HLEN + VLAN_HLEN) -#define MLX5E_XDP_TX_EMPTY_DS_COUNT \ - (sizeof(struct mlx5e_tx_wqe) / MLX5_SEND_WQE_DS) -#define MLX5E_XDP_TX_DS_COUNT (MLX5E_XDP_TX_EMPTY_DS_COUNT + 1 /* SG DS */) +#define MLX5E_XDP_TX_DS_COUNT (MLX5E_TX_WQE_EMPTY_DS_COUNT + 1 /* SG DS */) #define MLX5E_XDP_INLINE_WQE_MAX_DS_CNT 16 #define MLX5E_XDP_INLINE_WQE_SZ_THRSD \ @@ -123,23 +121,20 @@ static inline void mlx5e_xmit_xdp_doorbell(struct mlx5e_xdpsq *sq) /* Enable inline WQEs to shift some load from a congested HCA (HW) to * a less congested cpu (SW). */ -static inline void mlx5e_xdp_update_inline_state(struct mlx5e_xdpsq *sq) +static inline bool mlx5e_xdp_get_inline_state(struct mlx5e_xdpsq *sq, bool cur) { u16 outstanding = sq->xdpi_fifo_pc - sq->xdpi_fifo_cc; - struct mlx5e_xdp_mpwqe *session = &sq->mpwqe; #define MLX5E_XDP_INLINE_WATERMARK_LOW 10 #define MLX5E_XDP_INLINE_WATERMARK_HIGH 128 - if (session->inline_on) { - if (outstanding <= MLX5E_XDP_INLINE_WATERMARK_LOW) - session->inline_on = 0; - return; - } + if (cur && outstanding <= MLX5E_XDP_INLINE_WATERMARK_LOW) + return false; + + if (!cur && outstanding >= MLX5E_XDP_INLINE_WATERMARK_HIGH) + return true; - /* inline is false */ - if (outstanding >= MLX5E_XDP_INLINE_WATERMARK_HIGH) - session->inline_on = 1; + return cur; } static inline bool mlx5e_xdp_mpqwe_is_full(struct mlx5e_xdp_mpwqe *session) diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c index f967bc0573c0..46bdbbbfaf65 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c @@ -289,7 +289,7 @@ static inline void mlx5e_sq_calc_wqe_attr(struct sk_buff *skb, const struct mlx5e_tx_attr *attr, struct mlx5e_tx_wqe_attr *wqe_attr) { - u16 ds_cnt = sizeof(struct mlx5e_tx_wqe) / MLX5_SEND_WQE_DS; + u16 ds_cnt = MLX5E_TX_WQE_EMPTY_DS_COUNT; u16 ds_cnt_inl = 0; ds_cnt += !!attr->headlen + skb_shinfo(skb)->nr_frags;