| Message ID | 20200903210022.22774-10-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> > > mlx5e_txwqe_complete performs some actions that can be taken to separate > functions: > > 1. Update the flags needed for hardware timestamping. > > 2. Stop the TX queue if it's full. > > Take these actions into separate functions to be reused by the MPWQE > code in the following commit and to maintain clear responsibilities of > functions. > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > --- > .../net/ethernet/mellanox/mlx5/core/en_tx.c | 23 ++++++++++++++----- > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > index 9ced350150b3..3b68c8333875 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > @@ -311,6 +311,20 @@ static inline void mlx5e_sq_calc_wqe_attr(struct sk_buff *skb, > }; > } > > +static inline void mlx5e_tx_skb_update_hwts_flags(struct sk_buff *skb) > +{ > + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) > + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; > +} Subjective, but this helper adds a level of indirection and introduces code churn without simplying anything, imho. > +static inline void mlx5e_tx_check_stop(struct mlx5e_txqsq *sq) > +{ > + if (unlikely(!mlx5e_wqc_has_room_for(&sq->wq, sq->cc, sq->pc, sq->stop_room))) { > + netif_tx_stop_queue(sq->txq); > + sq->stats->stopped++; > + } > +} > + > static inline void > mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb, > const struct mlx5e_tx_attr *attr, > @@ -332,14 +346,11 @@ mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb, > cseg->opmod_idx_opcode = cpu_to_be32((sq->pc << 8) | attr->opcode); > cseg->qpn_ds = cpu_to_be32((sq->sqn << 8) | wqe_attr->ds_cnt); > > - if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) > - skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; > + mlx5e_tx_skb_update_hwts_flags(skb); > > sq->pc += wi->num_wqebbs; > - if (unlikely(!mlx5e_wqc_has_room_for(wq, sq->cc, sq->pc, sq->stop_room))) { > - netif_tx_stop_queue(sq->txq); > - sq->stats->stopped++; > - } > + > + mlx5e_tx_check_stop(sq); > > send_doorbell = __netdev_tx_sent_queue(sq->txq, attr->num_bytes, xmit_more); > if (send_doorbell) > -- > 2.26.2 >
On 2020-09-04 18:06, 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> >> >> mlx5e_txwqe_complete performs some actions that can be taken to separate >> functions: >> >> 1. Update the flags needed for hardware timestamping. >> >> 2. Stop the TX queue if it's full. >> >> Take these actions into separate functions to be reused by the MPWQE >> code in the following commit and to maintain clear responsibilities of >> functions. >> >> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> >> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> >> --- >> .../net/ethernet/mellanox/mlx5/core/en_tx.c | 23 ++++++++++++++----- >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c >> index 9ced350150b3..3b68c8333875 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c >> @@ -311,6 +311,20 @@ static inline void mlx5e_sq_calc_wqe_attr(struct sk_buff *skb, >> }; >> } >> >> +static inline void mlx5e_tx_skb_update_hwts_flags(struct sk_buff *skb) >> +{ >> + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) >> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; >> +} > > Subjective, but this helper adds a level of indirection and introduces > code churn without simplying anything, imho. It's added for the sake of being reused in non-MPWQE and MPWQE flows. >> +static inline void mlx5e_tx_check_stop(struct mlx5e_txqsq *sq) >> +{ >> + if (unlikely(!mlx5e_wqc_has_room_for(&sq->wq, sq->cc, sq->pc, sq->stop_room))) { >> + netif_tx_stop_queue(sq->txq); >> + sq->stats->stopped++; >> + } >> +} >> + >> static inline void >> mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb, >> const struct mlx5e_tx_attr *attr, >> @@ -332,14 +346,11 @@ mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb, >> cseg->opmod_idx_opcode = cpu_to_be32((sq->pc << 8) | attr->opcode); >> cseg->qpn_ds = cpu_to_be32((sq->sqn << 8) | wqe_attr->ds_cnt); >> >> - if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) >> - skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; >> + mlx5e_tx_skb_update_hwts_flags(skb); >> >> sq->pc += wi->num_wqebbs; >> - if (unlikely(!mlx5e_wqc_has_room_for(wq, sq->cc, sq->pc, sq->stop_room))) { >> - netif_tx_stop_queue(sq->txq); >> - sq->stats->stopped++; >> - } >> + >> + mlx5e_tx_check_stop(sq); >> >> send_doorbell = __netdev_tx_sent_queue(sq->txq, attr->num_bytes, xmit_more); >> if (send_doorbell) >> -- >> 2.26.2 >>
On Tue, Sep 8, 2020 at 11:00 AM Maxim Mikityanskiy <maximmi@nvidia.com> wrote: > > On 2020-09-04 18:06, 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> > >> > >> mlx5e_txwqe_complete performs some actions that can be taken to separate > >> functions: > >> > >> 1. Update the flags needed for hardware timestamping. > >> > >> 2. Stop the TX queue if it's full. > >> > >> Take these actions into separate functions to be reused by the MPWQE > >> code in the following commit and to maintain clear responsibilities of > >> functions. > >> > >> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > >> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > >> --- > >> .../net/ethernet/mellanox/mlx5/core/en_tx.c | 23 ++++++++++++++----- > >> 1 file changed, 17 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > >> index 9ced350150b3..3b68c8333875 100644 > >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > >> @@ -311,6 +311,20 @@ static inline void mlx5e_sq_calc_wqe_attr(struct sk_buff *skb, > >> }; > >> } > >> > >> +static inline void mlx5e_tx_skb_update_hwts_flags(struct sk_buff *skb) > >> +{ > >> + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) > >> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; > >> +} > > > > Subjective, but this helper adds a level of indirection and introduces > > code churn without simplying anything, imho. > > It's added for the sake of being reused in non-MPWQE and MPWQE flows. I understand. I'm just saying that a helper for two lines whose function is clear just adds a layer of obfuscation. As said, that is subjective, so just keep as is as you disagree.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c index 9ced350150b3..3b68c8333875 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c @@ -311,6 +311,20 @@ static inline void mlx5e_sq_calc_wqe_attr(struct sk_buff *skb, }; } +static inline void mlx5e_tx_skb_update_hwts_flags(struct sk_buff *skb) +{ + if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; +} + +static inline void mlx5e_tx_check_stop(struct mlx5e_txqsq *sq) +{ + if (unlikely(!mlx5e_wqc_has_room_for(&sq->wq, sq->cc, sq->pc, sq->stop_room))) { + netif_tx_stop_queue(sq->txq); + sq->stats->stopped++; + } +} + static inline void mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb, const struct mlx5e_tx_attr *attr, @@ -332,14 +346,11 @@ mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb, cseg->opmod_idx_opcode = cpu_to_be32((sq->pc << 8) | attr->opcode); cseg->qpn_ds = cpu_to_be32((sq->sqn << 8) | wqe_attr->ds_cnt); - if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) - skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS; + mlx5e_tx_skb_update_hwts_flags(skb); sq->pc += wi->num_wqebbs; - if (unlikely(!mlx5e_wqc_has_room_for(wq, sq->cc, sq->pc, sq->stop_room))) { - netif_tx_stop_queue(sq->txq); - sq->stats->stopped++; - } + + mlx5e_tx_check_stop(sq); send_doorbell = __netdev_tx_sent_queue(sq->txq, attr->num_bytes, xmit_more); if (send_doorbell)