diff mbox series

[net-next,V2,03/12] net/mlx5e: Move mlx5e_tx_wqe_inline_mode to en_tx.c

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

Commit Message

Saeed Mahameed Sept. 9, 2020, 1:27 a.m. UTC 
From: Maxim Mikityanskiy <maximmi@mellanox.com>

Move mlx5e_tx_wqe_inline_mode from en/txrx.h to en_tx.c as it's only
used there.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en/txrx.h | 23 -------------------
 .../net/ethernet/mellanox/mlx5/core/en_tx.c   | 23 +++++++++++++++++++
 2 files changed, 23 insertions(+), 23 deletions(-)

Comments

David Miller Sept. 9, 2020, 3:28 a.m. UTC | #1 
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.
David Miller Sept. 9, 2020, 3:29 a.m. UTC | #2 
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.
Saeed Mahameed Sept. 9, 2020, 7:22 p.m. UTC | #3 
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.
Jakub Kicinski Sept. 9, 2020, 7:28 p.m. UTC | #4 
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?
Saeed Mahameed Sept. 9, 2020, 8:48 p.m. UTC | #5 
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.
David Miller Sept. 9, 2020, 9:34 p.m. UTC | #6 
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.
Saeed Mahameed Sept. 11, 2020, 5:25 p.m. UTC | #7 
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 mbox series

Patch

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,