Message ID | 20200909012757.32677-4-saeedm@nvidia.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net-next,V2,01/12] net/mlx5e: Refactor inline header size calculation in the TX path | expand |
From: Saeed Mahameed <saeedm@nvidia.com> Date: Tue, 8 Sep 2020 18:27:48 -0700 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > @@ -232,6 +232,29 @@ mlx5e_txwqe_build_dsegs(struct mlx5e_txqsq *sq, struct sk_buff *skb, > return -ENOMEM; > } > > +static inline bool mlx5e_transport_inline_tx_wqe(struct mlx5_wqe_ctrl_seg *cseg) > +{ > + return cseg && !!cseg->tis_tir_num; > +} > + > +static inline u8 > +mlx5e_tx_wqe_inline_mode(struct mlx5e_txqsq *sq, struct mlx5_wqe_ctrl_seg *cseg, > + struct sk_buff *skb) > +{ No inlines in foo.c files, please.
From: David Miller <davem@davemloft.net> Date: Tue, 08 Sep 2020 20:28:36 -0700 (PDT) > From: Saeed Mahameed <saeedm@nvidia.com> > Date: Tue, 8 Sep 2020 18:27:48 -0700 > >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c >> @@ -232,6 +232,29 @@ mlx5e_txwqe_build_dsegs(struct mlx5e_txqsq *sq, struct sk_buff *skb, >> return -ENOMEM; >> } >> >> +static inline bool mlx5e_transport_inline_tx_wqe(struct mlx5_wqe_ctrl_seg *cseg) >> +{ >> + return cseg && !!cseg->tis_tir_num; >> +} >> + >> +static inline u8 >> +mlx5e_tx_wqe_inline_mode(struct mlx5e_txqsq *sq, struct mlx5_wqe_ctrl_seg *cseg, >> + struct sk_buff *skb) >> +{ > > No inlines in foo.c files, please. I see you're doing this even more later in this series. Please fix all of this up, thank you.
On Tue, 2020-09-08 at 20:29 -0700, David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Tue, 08 Sep 2020 20:28:36 -0700 (PDT) > > > From: Saeed Mahameed <saeedm@nvidia.com> > > Date: Tue, 8 Sep 2020 18:27:48 -0700 > > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > > > @@ -232,6 +232,29 @@ mlx5e_txwqe_build_dsegs(struct mlx5e_txqsq > > > *sq, struct sk_buff *skb, > > > return -ENOMEM; > > > } > > > > > > +static inline bool mlx5e_transport_inline_tx_wqe(struct > > > mlx5_wqe_ctrl_seg *cseg) > > > +{ > > > + return cseg && !!cseg->tis_tir_num; > > > +} > > > + > > > +static inline u8 > > > +mlx5e_tx_wqe_inline_mode(struct mlx5e_txqsq *sq, struct > > > mlx5_wqe_ctrl_seg *cseg, > > > + struct sk_buff *skb) > > > +{ > > > > No inlines in foo.c files, please. > > I see you're doing this even more later in this series. > > Please fix all of this up, thank you. Hi Dave, Maxim really tried here to avoid this without huge performance degradation (~6.4% reduce in packet rate), due to the refactoring patches gcc yields non optimal code, as we explained in the commit messages and cover-letter Our other option is making the code very ugly with no code reuse in the tx path, so we would really appreciate if you could relax the no-inline guideline for this series. Thanks, Saeed.
On Wed, 9 Sep 2020 19:22:02 +0000 Saeed Mahameed wrote: > On Tue, 2020-09-08 at 20:29 -0700, David Miller wrote: > > From: David Miller <davem@davemloft.net> > > Date: Tue, 08 Sep 2020 20:28:36 -0700 (PDT) > > > > > From: Saeed Mahameed <saeedm@nvidia.com> > > > Date: Tue, 8 Sep 2020 18:27:48 -0700 > > > > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > > > > @@ -232,6 +232,29 @@ mlx5e_txwqe_build_dsegs(struct mlx5e_txqsq > > > > *sq, struct sk_buff *skb, > > > > return -ENOMEM; > > > > } > > > > > > > > +static inline bool mlx5e_transport_inline_tx_wqe(struct > > > > mlx5_wqe_ctrl_seg *cseg) > > > > +{ > > > > + return cseg && !!cseg->tis_tir_num; > > > > +} > > > > + > > > > +static inline u8 > > > > +mlx5e_tx_wqe_inline_mode(struct mlx5e_txqsq *sq, struct > > > > mlx5_wqe_ctrl_seg *cseg, > > > > + struct sk_buff *skb) > > > > +{ > > > > > > No inlines in foo.c files, please. > > > > I see you're doing this even more later in this series. > > > > Please fix all of this up, thank you. > > Maxim really tried here to avoid this without huge performance > degradation (~6.4% reduce in packet rate), due to the refactoring > patches gcc yields non optimal code, as we explained in the commit > messages and cover-letter > > Our other option is making the code very ugly with no code reuse in the > tx path, so we would really appreciate if you could relax the no-inline > guideline for this series. Why are you requesting a whole pass on the series when only _some_ inlines make a difference here?
On Wed, 2020-09-09 at 12:28 -0700, Jakub Kicinski wrote: > On Wed, 9 Sep 2020 19:22:02 +0000 Saeed Mahameed wrote: > > On Tue, 2020-09-08 at 20:29 -0700, David Miller wrote: > > > From: David Miller <davem@davemloft.net> > > > Date: Tue, 08 Sep 2020 20:28:36 -0700 (PDT) > > > > > > > From: Saeed Mahameed <saeedm@nvidia.com> > > > > Date: Tue, 8 Sep 2020 18:27:48 -0700 > > > > > > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c > > > > > @@ -232,6 +232,29 @@ mlx5e_txwqe_build_dsegs(struct > > > > > mlx5e_txqsq > > > > > *sq, struct sk_buff *skb, > > > > > return -ENOMEM; > > > > > } > > > > > > > > > > +static inline bool mlx5e_transport_inline_tx_wqe(struct > > > > > mlx5_wqe_ctrl_seg *cseg) > > > > > +{ > > > > > + return cseg && !!cseg->tis_tir_num; > > > > > +} > > > > > + > > > > > +static inline u8 > > > > > +mlx5e_tx_wqe_inline_mode(struct mlx5e_txqsq *sq, struct > > > > > mlx5_wqe_ctrl_seg *cseg, > > > > > + struct sk_buff *skb) > > > > > +{ > > > > > > > > No inlines in foo.c files, please. > > > > > > I see you're doing this even more later in this series. > > > > > > Please fix all of this up, thank you. > > > > Maxim really tried here to avoid this without huge performance > > degradation (~6.4% reduce in packet rate), due to the refactoring > > patches gcc yields non optimal code, as we explained in the commit > > messages and cover-letter > > > > Our other option is making the code very ugly with no code reuse in > > the > > tx path, so we would really appreciate if you could relax the no- > > inline > > guideline for this series. > > Why are you requesting a whole pass on the series when only _some_ > inlines make a difference here? I meant for the inilines that are necessary to avoid the performance drop, Maxim can make the change and remove the unnecessary inline functions if it is ok with You and Dave.
From: Saeed Mahameed <saeedm@nvidia.com> Date: Wed, 9 Sep 2020 19:22:02 +0000 > Maxim really tried here to avoid this without huge performance > degradation (~6.4% reduce in packet rate), due to the refactoring > patches gcc yields non optimal code, as we explained in the commit > messages and cover-letter > > Our other option is making the code very ugly with no code reuse in the > tx path, so we would really appreciate if you could relax the no-inline > guideline for this series. Submit a compiler bug report. I'm standing firm on our policy. If you don't follow it, there is zero incentive to get the compiler fixed, which cures the problem in one place and for everyone rather than just your special case. Thanks.
On Wed, 2020-09-09 at 14:34 -0700, David Miller wrote: > From: Saeed Mahameed <saeedm@nvidia.com> > Date: Wed, 9 Sep 2020 19:22:02 +0000 > > > Maxim really tried here to avoid this without huge performance > > degradation (~6.4% reduce in packet rate), due to the refactoring > > patches gcc yields non optimal code, as we explained in the commit > > messages and cover-letter > > > > Our other option is making the code very ugly with no code reuse in > > the > > tx path, so we would really appreciate if you could relax the no- > > inline > > guideline for this series. > > Submit a compiler bug report. > > I'm standing firm on our policy. If you don't follow it, there is > zero > incentive to get the compiler fixed, which cures the problem in one > place and for everyone rather than just your special case. > Ack, Maxim will be testing several versions of GCC to compare and understand the behavior better and will resubmit a version with no manual inline hints later, for now i would like to put this on-hold and send the next pull request. Thanks, Saeed.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h index 9334c9c3e208..6be04a236017 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h @@ -223,29 +223,6 @@ mlx5e_notify_hw(struct mlx5_wq_cyc *wq, u16 pc, void __iomem *uar_map, mlx5_write64((__be32 *)ctrl, uar_map); } -static inline bool mlx5e_transport_inline_tx_wqe(struct mlx5_wqe_ctrl_seg *cseg) -{ - return cseg && !!cseg->tis_tir_num; -} - -static inline u8 -mlx5e_tx_wqe_inline_mode(struct mlx5e_txqsq *sq, struct mlx5_wqe_ctrl_seg *cseg, - struct sk_buff *skb) -{ - u8 mode; - - if (mlx5e_transport_inline_tx_wqe(cseg)) - return MLX5_INLINE_MODE_TCP_UDP; - - mode = sq->min_inline_mode; - - if (skb_vlan_tag_present(skb) && - test_bit(MLX5E_SQ_STATE_VLAN_NEED_L2_INLINE, &sq->state)) - mode = max_t(u8, MLX5_INLINE_MODE_L2, mode); - - return mode; -} - static inline void mlx5e_cq_arm(struct mlx5e_cq *cq) { struct mlx5_core_cq *mcq; diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c index c064657dde13..be76ae14d186 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c @@ -232,6 +232,29 @@ mlx5e_txwqe_build_dsegs(struct mlx5e_txqsq *sq, struct sk_buff *skb, return -ENOMEM; } +static inline bool mlx5e_transport_inline_tx_wqe(struct mlx5_wqe_ctrl_seg *cseg) +{ + return cseg && !!cseg->tis_tir_num; +} + +static inline u8 +mlx5e_tx_wqe_inline_mode(struct mlx5e_txqsq *sq, struct mlx5_wqe_ctrl_seg *cseg, + struct sk_buff *skb) +{ + u8 mode; + + if (mlx5e_transport_inline_tx_wqe(cseg)) + return MLX5_INLINE_MODE_TCP_UDP; + + mode = sq->min_inline_mode; + + if (skb_vlan_tag_present(skb) && + test_bit(MLX5E_SQ_STATE_VLAN_NEED_L2_INLINE, &sq->state)) + mode = max_t(u8, MLX5_INLINE_MODE_L2, mode); + + return mode; +} + static inline void mlx5e_txwqe_complete(struct mlx5e_txqsq *sq, struct sk_buff *skb, u8 opcode, u16 ds_cnt, u8 num_wqebbs, u32 num_bytes, u8 num_dma,