[PATCH v2 net-next] net: stmmac: add drop transmit status feature

[PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Joao Pinto]

When the Drop Transmit Status bit is set, the Tx packet status
received from the MAC is dropped in the MTL. When this bit is reset,
the Tx packet status received from the MAC is forwarded to the
application. This feature will cause a performance improvement.

Signed-off-by: Joao Pinto <jpinto@synopsys.com>
---
changes v1->v2:
- removed mask from dwmac4_enable_tx_drop()

 Documentation/devicetree/bindings/net/stmmac.txt      |  1 +
 drivers/net/ethernet/stmicro/stmmac/common.h          |  2 ++
 drivers/net/ethernet/stmicro/stmmac/dwmac4.h          |  1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c     | 12 ++++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     |  3 +++
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  2 ++
 include/linux/stmmac.h                                |  1 +
 7 files changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
index f652b0c..dbcb2cc 100644
--- a/Documentation/devicetree/bindings/net/stmmac.txt
+++ b/Documentation/devicetree/bindings/net/stmmac.txt
@@ -60,6 +60,7 @@ Optional properties:
 		 and MAC2MAC connection.
 - snps,tso: this enables the TSO feature otherwise it will be managed by
 		 MAC HW capability register. Only for GMAC4 and newer.
+- snps,drop-tx-status: this enables drop tx status
 - AXI BUS Mode parameters: below the list of all the parameters to program the
 			   AXI register inside the DMA module:
 	- snps,lpi_en: enable Low Power Interface
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 90d28bc..312f9670 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -487,6 +487,8 @@ struct stmmac_ops {
 	/* RX Queues Routing */
 	void (*rx_queue_routing)(struct mac_device_info *hw, u8 packet,
 				 u32 queue);
+	/* Enable TX drop */
+	void (*enable_tx_drop)(struct mac_device_info *hw);
 	/* Program RX Algorithms */
 	void (*prog_mtl_rx_algorithms)(struct mac_device_info *hw, u32 rx_alg);
 	/* Program TX Algorithms */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
index d74cedf..56ba01a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
@@ -202,6 +202,7 @@ enum power_event {
 #define MTL_OPERATION_RAA		BIT(2)
 #define MTL_OPERATION_RAA_SP		(0x0 << 2)
 #define MTL_OPERATION_RAA_WSP		(0x1 << 2)
+#define MTL_OPERATION_DTXSTS		BIT(1)
 
 #define MTL_INT_STATUS			0x00000c20
 #define MTL_INT_QX(x)			BIT(x)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index 48793f2..e9bc51c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -142,6 +142,16 @@ static void dwmac4_tx_queue_routing(struct mac_device_info *hw,
 	writel(value, ioaddr + GMAC_RXQ_CTRL1);
 }
 
+static void dwmac4_enable_tx_drop(struct mac_device_info *hw)
+{
+	void __iomem *ioaddr = hw->pcsr;
+	u32 value = readl(ioaddr + MTL_OPERATION_MODE);
+
+	value |= MTL_OPERATION_DTXSTS;
+
+	writel(value, ioaddr + MTL_OPERATION_MODE);
+}
+
 static void dwmac4_prog_mtl_rx_algorithms(struct mac_device_info *hw,
 					  u32 rx_alg)
 {
@@ -675,6 +685,7 @@ static const struct stmmac_ops dwmac4_ops = {
 	.rx_queue_prio = dwmac4_rx_queue_priority,
 	.tx_queue_prio = dwmac4_tx_queue_priority,
 	.rx_queue_routing = dwmac4_tx_queue_routing,
+	.enable_tx_drop = dwmac4_enable_tx_drop,
 	.prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms,
 	.prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms,
 	.set_mtl_tx_queue_weight = dwmac4_set_mtl_tx_queue_weight,
@@ -706,6 +717,7 @@ static const struct stmmac_ops dwmac410_ops = {
 	.rx_queue_prio = dwmac4_rx_queue_priority,
 	.tx_queue_prio = dwmac4_tx_queue_priority,
 	.rx_queue_routing = dwmac4_tx_queue_routing,
+	.enable_tx_drop = dwmac4_enable_tx_drop,
 	.prog_mtl_rx_algorithms = dwmac4_prog_mtl_rx_algorithms,
 	.prog_mtl_tx_algorithms = dwmac4_prog_mtl_tx_algorithms,
 	.set_mtl_tx_queue_weight = dwmac4_set_mtl_tx_queue_weight,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index a89f76b..bbde5b2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2368,6 +2368,9 @@ static void stmmac_mtl_configuration(struct stmmac_priv *priv)
 	u32 rx_queues_count = priv->plat->rx_queues_to_use;
 	u32 tx_queues_count = priv->plat->tx_queues_to_use;
 
+	if (priv->plat->drop_tx_status)
+		priv->hw->mac->enable_tx_drop(priv->hw);
+
 	if (tx_queues_count > 1 && priv->hw->mac->set_mtl_tx_queue_weight)
 		stmmac_set_tx_queue_weight(priv);
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 7fc3a1e..d15e650 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -451,6 +451,8 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
 		plat->has_gmac = 0;
 		plat->pmt = 1;
 		plat->tso_en = of_property_read_bool(np, "snps,tso");
+		plat->drop_tx_status =
+			of_property_read_bool(np, "snps,drop-tx-status");
 	}
 
 	if (of_device_is_compatible(np, "snps,dwmac-3.610") ||
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 3921cb9..fe7940c 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -186,6 +186,7 @@ struct plat_stmmacenet_data {
 	struct stmmac_axi *axi;
 	int has_gmac4;
 	bool tso_en;
+	bool drop_tx_status;
 	int mac_port_sel_speed;
 	bool en_tx_lpi_clockgating;
 };
-- 
2.9.3

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Andrew Lunn]

On Wed, Apr 12, 2017 at 10:26:20AM +0100, Joao Pinto wrote:
> When the Drop Transmit Status bit is set, the Tx packet status
> received from the MAC is dropped in the MTL. When this bit is reset,
> the Tx packet status received from the MAC is forwarded to the
> application. This feature will cause a performance improvement.
> 
> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> ---
> changes v1->v2:
> - removed mask from dwmac4_enable_tx_drop()
> 
>  Documentation/devicetree/bindings/net/stmmac.txt      |  1 +
>  drivers/net/ethernet/stmicro/stmmac/common.h          |  2 ++
>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h          |  1 +
>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c     | 12 ++++++++++++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     |  3 +++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  2 ++
>  include/linux/stmmac.h                                |  1 +
>  7 files changed, 22 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
> index f652b0c..dbcb2cc 100644
> --- a/Documentation/devicetree/bindings/net/stmmac.txt
> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
> @@ -60,6 +60,7 @@ Optional properties:
>  		 and MAC2MAC connection.
>  - snps,tso: this enables the TSO feature otherwise it will be managed by
>  		 MAC HW capability register. Only for GMAC4 and newer.
> +- snps,drop-tx-status: this enables drop tx status

Hi Joao

Was the conclusion from testing that this cannot be turned on by
default?

What sort of performance improvement did you get? Do you have some
benchmark numbers?

	  Andrew

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Joao Pinto]

Hi Andrew,

Às 12:56 PM de 4/12/2017, Andrew Lunn escreveu:
> On Wed, Apr 12, 2017 at 10:26:20AM +0100, Joao Pinto wrote:
>> When the Drop Transmit Status bit is set, the Tx packet status
>> received from the MAC is dropped in the MTL. When this bit is reset,
>> the Tx packet status received from the MAC is forwarded to the
>> application. This feature will cause a performance improvement.
>>
>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>> ---
>> changes v1->v2:
>> - removed mask from dwmac4_enable_tx_drop()
>>
>>  Documentation/devicetree/bindings/net/stmmac.txt      |  1 +
>>  drivers/net/ethernet/stmicro/stmmac/common.h          |  2 ++
>>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h          |  1 +
>>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c     | 12 ++++++++++++
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     |  3 +++
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  2 ++
>>  include/linux/stmmac.h                                |  1 +
>>  7 files changed, 22 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
>> index f652b0c..dbcb2cc 100644
>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>> @@ -60,6 +60,7 @@ Optional properties:
>>  		 and MAC2MAC connection.
>>  - snps,tso: this enables the TSO feature otherwise it will be managed by
>>  		 MAC HW capability register. Only for GMAC4 and newer.
>> +- snps,drop-tx-status: this enables drop tx status
> 
> Hi Joao
> 
> Was the conclusion from testing that this cannot be turned on by
> default?

This feature is great for applications that need good performance, but has a
drawback since it has an impact in timestamp feature in Tx. There are some
operations in PTP where the timestamp is given to the host through the TX status
in the descriptor, so this will have an impact.

There's a way of solving this of course by making the driver checking the
timestamp in the MAC_Tx_Timestamp_Status_XXX registers, but I can only look into
that feature later in the future.

> 
> What sort of performance improvement did you get? Do you have some
> benchmark numbers?

My setup is FPGA based, so it will have lower performance values.
Iperf results with
  "Drop Transmit Status" set: ~650Mbps.
  "Drop Transmit Status" unset: ~450Mbps.

> 
> 	  Andrew
> 

Joao

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Andrew Lunn]

> >> +- snps,drop-tx-status: this enables drop tx status
> > 
> > Hi Joao
> > 
> > Was the conclusion from testing that this cannot be turned on by
> > default?
> 
> This feature is great for applications that need good performance, but has a
> drawback since it has an impact in timestamp feature in Tx. There are some
> operations in PTP where the timestamp is given to the host through the TX status
> in the descriptor, so this will have an impact.
> 
> There's a way of solving this of course by making the driver checking the
> timestamp in the MAC_Tx_Timestamp_Status_XXX registers, but I can only look into
> that feature later in the future.

The problem you have is that the device tree binding is a Binary API
you have to keep backwards compatible with for the next 20 years. You
cannot drop this property when you do get around to finishing the
work. You also want to avoid adding more and more options, which
nobody knows what they do, and what best combination is to get the
best performance. You should be aiming for a driver which just works
without any configuration and with good performance.

> > What sort of performance improvement did you get? Do you have some
> > benchmark numbers?
> 
> My setup is FPGA based, so it will have lower performance values.
> Iperf results with
>   "Drop Transmit Status" set: ~650Mbps.
>   "Drop Transmit Status" unset: ~450Mbps.

What percentage of your customers use FPGAs? When i look at the users
of this driver, i see ST, Allwinner, Rockchip, Meson, etc. So silicon,
not FPGA. Does it make sense to do performance measurements on FPGA,
when you say it has lower performance?

     Andrew

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Joao Pinto]

Às 2:10 PM de 4/12/2017, Andrew Lunn escreveu:
>>>> +- snps,drop-tx-status: this enables drop tx status
>>>
>>> Hi Joao
>>>
>>> Was the conclusion from testing that this cannot be turned on by
>>> default?
>>
>> This feature is great for applications that need good performance, but has a
>> drawback since it has an impact in timestamp feature in Tx. There are some
>> operations in PTP where the timestamp is given to the host through the TX status
>> in the descriptor, so this will have an impact.
>>
>> There's a way of solving this of course by making the driver checking the
>> timestamp in the MAC_Tx_Timestamp_Status_XXX registers, but I can only look into
>> that feature later in the future.
> 
> The problem you have is that the device tree binding is a Binary API
> you have to keep backwards compatible with for the next 20 years. You
> cannot drop this property when you do get around to finishing the
> work. You also want to avoid adding more and more options, which
> nobody knows what they do, and what best combination is to get the
> best performance. You should be aiming for a driver which just works
> without any configuration and with good performance.
> 
>>> What sort of performance improvement did you get? Do you have some
>>> benchmark numbers?
>>
>> My setup is FPGA based, so it will have lower performance values.
>> Iperf results with
>>   "Drop Transmit Status" set: ~650Mbps.
>>   "Drop Transmit Status" unset: ~450Mbps.
> 
> What percentage of your customers use FPGAs? When i look at the users
> of this driver, i see ST, Allwinner, Rockchip, Meson, etc. So silicon,
> not FPGA. Does it make sense to do performance measurements on FPGA,
> when you say it has lower performance?

I don't understand your question. Synopsys is an IP vendor, so all recent IPs
are available for prototyping as you can understand and so early development is
done using a FPGA.

I only mentioned that the values were from a FPGA based setup because you could
think that they were low. Performance values are just an indication.

> 
>      Andrew
> 

Joao

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Andrew Lunn]

> >> My setup is FPGA based, so it will have lower performance values.
> >> Iperf results with
> >>   "Drop Transmit Status" set: ~650Mbps.
> >>   "Drop Transmit Status" unset: ~450Mbps.
> > 
> > What percentage of your customers use FPGAs? When i look at the users
> > of this driver, i see ST, Allwinner, Rockchip, Meson, etc. So silicon,
> > not FPGA. Does it make sense to do performance measurements on FPGA,
> > when you say it has lower performance?
> 
> I don't understand your question. Synopsys is an IP vendor, so all recent IPs
> are available for prototyping as you can understand and so early development is
> done using a FPGA.

Sure, early development on FPGA makes sense. But my guess is, > 95% of
the devices running this driver are silicon, not FPGA. The customers
you sell the IP to want to know that the driver is going to work well
on their silicon, not your internal FPGA development setup. The kernel
community want a warm fuzzy feeling that you care about the real
devices out in the wild using this driver. So if you could say, "I
tested on a STM DISCO devel board, and performance went up 20%, an
RK3288 devel board and got 18% performance boost, and a Merrii A80
Optimus Board showed 22% improvements", we would have a lot better
feeling about these patches.

	Andrew

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Joao Pinto]

Às 2:52 PM de 4/12/2017, Andrew Lunn escreveu:
>>>> My setup is FPGA based, so it will have lower performance values.
>>>> Iperf results with
>>>>   "Drop Transmit Status" set: ~650Mbps.
>>>>   "Drop Transmit Status" unset: ~450Mbps.
>>>
>>> What percentage of your customers use FPGAs? When i look at the users
>>> of this driver, i see ST, Allwinner, Rockchip, Meson, etc. So silicon,
>>> not FPGA. Does it make sense to do performance measurements on FPGA,
>>> when you say it has lower performance?
>>
>> I don't understand your question. Synopsys is an IP vendor, so all recent IPs
>> are available for prototyping as you can understand and so early development is
>> done using a FPGA.
> 
> Sure, early development on FPGA makes sense. But my guess is, > 95% of
> the devices running this driver are silicon, not FPGA. The customers
> you sell the IP to want to know that the driver is going to work well
> on their silicon, not your internal FPGA development setup. The kernel
> community want a warm fuzzy feeling that you care about the real
> devices out in the wild using this driver. So if you could say, "I
> tested on a STM DISCO devel board, and performance went up 20%, an
> RK3288 devel board and got 18% performance boost, and a Merrii A80
> Optimus Board showed 22% improvements", we would have a lot better
> feeling about these patches.

Understand your point, but for now our development and testing setup will be
based on the IP Prototyping Kit, consisting of a FPGA + PHY.

Thanks,
Joao

> 
> 	Andrew
> 

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[David Miller]

From: Joao Pinto <Joao.Pinto@synopsys.com>
Date: Wed, 12 Apr 2017 14:55:03 +0100

> Understand your point, but for now our development and testing setup will be
> based on the IP Prototyping Kit, consisting of a FPGA + PHY.

That's completely, and utterly, unacceptable.

I will be quite frank with you, that instances like this are causing
people to contact me privately and telling me that your handling of
becomming the stmmac driver maintainer is causing very real and
serious concerns.

You cannot develop performance based features and only test their
impact on FPGA when almost all users are on real silicon.

And this requirement is absolutely non-negotiable.

You must test the impact on real silicon otherwise your performance
numbers, which are required to be provided in the commit message
for any "performance" feature or change, are completely useless.

I want your attitude on these matters to change quickly, as myself
and many other interested parties are becomming extremely frustrated
with how you are handling things.

Thank you.

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Andrew Lunn]

On Wed, Apr 12, 2017 at 01:07:02PM +0100, Joao Pinto wrote:
> Hi Andrew,
> 
> Às 12:56 PM de 4/12/2017, Andrew Lunn escreveu:
> > On Wed, Apr 12, 2017 at 10:26:20AM +0100, Joao Pinto wrote:
> >> When the Drop Transmit Status bit is set, the Tx packet status
> >> received from the MAC is dropped in the MTL. When this bit is reset,
> >> the Tx packet status received from the MAC is forwarded to the
> >> application. This feature will cause a performance improvement.
> >>
> >> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> >> ---
> >> changes v1->v2:
> >> - removed mask from dwmac4_enable_tx_drop()
> >>
> >>  Documentation/devicetree/bindings/net/stmmac.txt      |  1 +
> >>  drivers/net/ethernet/stmicro/stmmac/common.h          |  2 ++
> >>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h          |  1 +
> >>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c     | 12 ++++++++++++
> >>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     |  3 +++
> >>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  2 ++
> >>  include/linux/stmmac.h                                |  1 +
> >>  7 files changed, 22 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
> >> index f652b0c..dbcb2cc 100644
> >> --- a/Documentation/devicetree/bindings/net/stmmac.txt
> >> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
> >> @@ -60,6 +60,7 @@ Optional properties:
> >>  		 and MAC2MAC connection.
> >>  - snps,tso: this enables the TSO feature otherwise it will be managed by
> >>  		 MAC HW capability register. Only for GMAC4 and newer.
> >> +- snps,drop-tx-status: this enables drop tx status
> > 
> > Hi Joao
> > 
> > Was the conclusion from testing that this cannot be turned on by
> > default?
> 
> This feature is great for applications that need good performance, but has a
> drawback since it has an impact in timestamp feature in Tx. There are some
> operations in PTP where the timestamp is given to the host through the TX status
> in the descriptor, so this will have an impact.
> 
> There's a way of solving this of course by making the driver checking the
> timestamp in the MAC_Tx_Timestamp_Status_XXX registers, but I can only look into
> that feature later in the future.

So the numbers you show, even if they are not on real hardware that
anybody uses, do look good.

But i don't like this DT property, it sounds like it can potential do
bad things to PTP, and you say it can be done without the DT or PTP
problems. So I would personally NACK this patch, and ask you to
re-submit when you have solved these issues.

But i don't use this driver, don't have any hardware, and i'm not a
maintainer for it....

    Andrew

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Joao Pinto]

Hello,

Às 3:51 PM de 4/12/2017, David Miller escreveu:
> From: Joao Pinto <Joao.Pinto@synopsys.com>
> Date: Wed, 12 Apr 2017 14:55:03 +0100
> 
>> Understand your point, but for now our development and testing setup will be
>> based on the IP Prototyping Kit, consisting of a FPGA + PHY.
> 
> That's completely, and utterly, unacceptable.
> 
> I will be quite frank with you, that instances like this are causing
> people to contact me privately and telling me that your handling of
> becomming the stmmac driver maintainer is causing very real and
> serious concerns.

So, adding features to the driver are causing concerns to people? People don't
have to worry about maintenance, since goals are not to be a maintainer. My job
is just to help improve and add missing features to the driver and that's it.

> 
> You cannot develop performance based features and only test their
> impact on FPGA when almost all users are on real silicon.
> 
> And this requirement is absolutely non-negotiable.
> 
> You must test the impact on real silicon otherwise your performance
> numbers, which are required to be provided in the commit message
> for any "performance" feature or change, are completely useless.

Next time I won't mention anything about performance, honestly. "Drop TX Status"
is just an IP Core feature that can or not be used, it is up to the driver user.

> 
> I want your attitude on these matters to change quickly, as myself
> and many other interested parties are becomming extremely frustrated
> with how you are handling things.

Attitude? Are there complaints with my attitude? I try to help in everything I
can, I listen to feedback, I did the rework on the Multiple Buffers patch to try
to solve the negative impact in sunxi board. Sincerely I don't see a bad attitude.

Are you mentioning the e-mail about sxgbe?

> 
> Thank you.
> 

Thank you.

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[David Miller]

From: Joao Pinto <Joao.Pinto@synopsys.com>
Date: Wed, 12 Apr 2017 16:13:33 +0100

> Às 3:51 PM de 4/12/2017, David Miller escreveu:
>> You cannot develop performance based features and only test their
>> impact on FPGA when almost all users are on real silicon.
>> 
>> And this requirement is absolutely non-negotiable.
>> 
>> You must test the impact on real silicon otherwise your performance
>> numbers, which are required to be provided in the commit message
>> for any "performance" feature or change, are completely useless.
> 
> Next time I won't mention anything about performance, honestly. "Drop TX Status"
> is just an IP Core feature that can or not be used, it is up to the driver user.

Being dishonest about why a change might be desirable doesn't help things, in fact
now that you've stated this intent in the future, people know to be suspucious of
your changes.

I seriously don't think you realize the ramifications of what you just said right
there.

Everything here is about trust, and if you create a situation where you can't be
trusted then the process of doing upstream development will be extremely difficult
and time consuming for you.

Thanks.

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Joao Pinto]

Às 4:28 PM de 4/12/2017, David Miller escreveu:
> From: Joao Pinto <Joao.Pinto@synopsys.com>
> Date: Wed, 12 Apr 2017 16:13:33 +0100
> 
>> Às 3:51 PM de 4/12/2017, David Miller escreveu:
>>> You cannot develop performance based features and only test their
>>> impact on FPGA when almost all users are on real silicon.
>>>
>>> And this requirement is absolutely non-negotiable.
>>>
>>> You must test the impact on real silicon otherwise your performance
>>> numbers, which are required to be provided in the commit message
>>> for any "performance" feature or change, are completely useless.
>>
>> Next time I won't mention anything about performance, honestly. "Drop TX Status"
>> is just an IP Core feature that can or not be used, it is up to the driver user.
> 
> Being dishonest about why a change might be desirable doesn't help things, in fact
> now that you've stated this intent in the future, people know to be suspucious of
> your changes.

Dishonest? I just sent the patch adding a optional configuration that can boost
performance in applications where timestapping is not an issue. You can request
more info in stmmac.txt, but calling me dishonest is a bit out of line.

I perfectly accept if you feel that the patch is not useful, that's fine.

> 
> I seriously don't think you realize the ramifications of what you just said right
> there.

No, I don't see honestly. I just said that I am a developer that has an interest
in the success of stmmac, but I don't want to steal maintenance seats :).

> 
> Everything here is about trust, and if you create a situation where you can't be
> trusted then the process of doing upstream development will be extremely difficult
> and time consuming for you.

Agree, trust is fundamental. I never gave reasons not to be trusted, in fact I
have a good relation with some of the stmmac developers and in other subsystems,
so I don't see the point of your observations.

> 
> Thanks.
> 

Joao

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[David Miller]

From: Joao Pinto <Joao.Pinto@synopsys.com>
Date: Wed, 12 Apr 2017 16:43:38 +0100

> Às 4:28 PM de 4/12/2017, David Miller escreveu:
>> From: Joao Pinto <Joao.Pinto@synopsys.com>
>> Date: Wed, 12 Apr 2017 16:13:33 +0100
>> 
>>> Às 3:51 PM de 4/12/2017, David Miller escreveu:
>>>> You cannot develop performance based features and only test their
>>>> impact on FPGA when almost all users are on real silicon.
>>>>
>>>> And this requirement is absolutely non-negotiable.
>>>>
>>>> You must test the impact on real silicon otherwise your performance
>>>> numbers, which are required to be provided in the commit message
>>>> for any "performance" feature or change, are completely useless.
>>>
>>> Next time I won't mention anything about performance, honestly. "Drop TX Status"
>>> is just an IP Core feature that can or not be used, it is up to the driver user.
>> 
>> Being dishonest about why a change might be desirable doesn't help things, in fact
>> now that you've stated this intent in the future, people know to be suspucious of
>> your changes.
> 
> Dishonest? I just sent the patch adding a optional configuration that can boost
> performance in applications where timestapping is not an issue. You can request
> more info in stmmac.txt, but calling me dishonest is a bit out of line.

If performance is a primary, if not the only, reason to make this
configuration setting then not mentioning it definitely falls into
the category of "sin of omission."

Please stop trying to weasel word your way out of this situation.

You're not interacting with the community properly, and your
unwillingness to do simple things people ask of you wrt. patch
submission and testing is upsetting a lot of people.

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Florian Fainelli]

On 04/12/2017 07:58 AM, Andrew Lunn wrote:
> On Wed, Apr 12, 2017 at 01:07:02PM +0100, Joao Pinto wrote:
>> Hi Andrew,
>>
>> Às 12:56 PM de 4/12/2017, Andrew Lunn escreveu:
>>> On Wed, Apr 12, 2017 at 10:26:20AM +0100, Joao Pinto wrote:
>>>> When the Drop Transmit Status bit is set, the Tx packet status
>>>> received from the MAC is dropped in the MTL. When this bit is reset,
>>>> the Tx packet status received from the MAC is forwarded to the
>>>> application. This feature will cause a performance improvement.
>>>>
>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>>>> ---
>>>> changes v1->v2:
>>>> - removed mask from dwmac4_enable_tx_drop()
>>>>
>>>>  Documentation/devicetree/bindings/net/stmmac.txt      |  1 +
>>>>  drivers/net/ethernet/stmicro/stmmac/common.h          |  2 ++
>>>>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h          |  1 +
>>>>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c     | 12 ++++++++++++
>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     |  3 +++
>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  2 ++
>>>>  include/linux/stmmac.h                                |  1 +
>>>>  7 files changed, 22 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
>>>> index f652b0c..dbcb2cc 100644
>>>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>>>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>>>> @@ -60,6 +60,7 @@ Optional properties:
>>>>  		 and MAC2MAC connection.
>>>>  - snps,tso: this enables the TSO feature otherwise it will be managed by
>>>>  		 MAC HW capability register. Only for GMAC4 and newer.
>>>> +- snps,drop-tx-status: this enables drop tx status
>>>
>>> Hi Joao
>>>
>>> Was the conclusion from testing that this cannot be turned on by
>>> default?
>>
>> This feature is great for applications that need good performance, but has a
>> drawback since it has an impact in timestamp feature in Tx. There are some
>> operations in PTP where the timestamp is given to the host through the TX status
>> in the descriptor, so this will have an impact.
>>
>> There's a way of solving this of course by making the driver checking the
>> timestamp in the MAC_Tx_Timestamp_Status_XXX registers, but I can only look into
>> that feature later in the future.
> 
> So the numbers you show, even if they are not on real hardware that
> anybody uses, do look good.
> 
> But i don't like this DT property, it sounds like it can potential do
> bad things to PTP, and you say it can be done without the DT or PTP
> problems. So I would personally NACK this patch, and ask you to
> re-submit when you have solved these issues.

Is not it possible to simply figure out whether this can safely be
enabled at run time or not? If nobody uses PTP, then turn that on as
soon as you get someone using PTP, turn this back off?

The problem is the commit message is extremely scarce about what this
implies, and quite frankly, I don't understand what "application" means
here, whether this means:

- application in the sense of the HW IP, which is then, the OS'
networking stack presumably?

- application in the sense of the OS, which is sort of conflated with
the socket user?

Worst case, can't you create a new netdev feature (e.g: similar to
NETIF_F_RXFCS) that allows dynamically turning this on based on user
configuration? Not suggesting this is the right approach here, but it
should be thought of. Maybe an ethtool private flag is more appropriate?

One perspective you need to look at is the following:

- as an IP vendor, you would like to exercise every little configuration
knob that the IP can be synthesized with, and that's fine and fun to do
as long as you are in your controlled environment, this may result in
custom DT properties, but those may not be candidate for the upstream
kernel (just downstream)

- customers want something that works out of the box, don't require
reading a DT binding (in fact, not reading at all is best) and just get
the promised performance, and the upstream kernel is the best place to
get this done, but it needs to be painless

Finally, and completely tangential to this patch, pretty much *every*
SoC that has a STMMAC is reasonably cheap to buy, so if Synopsys does
not, you could, with your own personal money acquire a full set of
Allwinner, Meson, Sunxi and what not platforms and put these in a test
labs, heck, I'm sure someone could offer these. Most maintainers just
collect HW as well as tribal knowledge about what typically breaks on
this or that platform, and then run their tests on said platforms.

Hope this helps!
-- 
Florian

Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature

[Joao Pinto]

Hi Florian,

Às 6:43 PM de 4/12/2017, Florian Fainelli escreveu:
> On 04/12/2017 07:58 AM, Andrew Lunn wrote:
>> On Wed, Apr 12, 2017 at 01:07:02PM +0100, Joao Pinto wrote:
>>> Hi Andrew,
>>>
>>> Às 12:56 PM de 4/12/2017, Andrew Lunn escreveu:
>>>> On Wed, Apr 12, 2017 at 10:26:20AM +0100, Joao Pinto wrote:
>>>>> When the Drop Transmit Status bit is set, the Tx packet status
>>>>> received from the MAC is dropped in the MTL. When this bit is reset,
>>>>> the Tx packet status received from the MAC is forwarded to the
>>>>> application. This feature will cause a performance improvement.
>>>>>
>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>>>>> ---
>>>>> changes v1->v2:
>>>>> - removed mask from dwmac4_enable_tx_drop()
>>>>>
>>>>>  Documentation/devicetree/bindings/net/stmmac.txt      |  1 +
>>>>>  drivers/net/ethernet/stmicro/stmmac/common.h          |  2 ++
>>>>>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h          |  1 +
>>>>>  drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c     | 12 ++++++++++++
>>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     |  3 +++
>>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  2 ++
>>>>>  include/linux/stmmac.h                                |  1 +
>>>>>  7 files changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
>>>>> index f652b0c..dbcb2cc 100644
>>>>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>>>>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>>>>> @@ -60,6 +60,7 @@ Optional properties:
>>>>>  		 and MAC2MAC connection.
>>>>>  - snps,tso: this enables the TSO feature otherwise it will be managed by
>>>>>  		 MAC HW capability register. Only for GMAC4 and newer.
>>>>> +- snps,drop-tx-status: this enables drop tx status
>>>>
>>>> Hi Joao
>>>>
>>>> Was the conclusion from testing that this cannot be turned on by
>>>> default?
>>>
>>> This feature is great for applications that need good performance, but has a
>>> drawback since it has an impact in timestamp feature in Tx. There are some
>>> operations in PTP where the timestamp is given to the host through the TX status
>>> in the descriptor, so this will have an impact.
>>>
>>> There's a way of solving this of course by making the driver checking the
>>> timestamp in the MAC_Tx_Timestamp_Status_XXX registers, but I can only look into
>>> that feature later in the future.
>>
>> So the numbers you show, even if they are not on real hardware that
>> anybody uses, do look good.
>>
>> But i don't like this DT property, it sounds like it can potential do
>> bad things to PTP, and you say it can be done without the DT or PTP
>> problems. So I would personally NACK this patch, and ask you to
>> re-submit when you have solved these issues.
> 
> Is not it possible to simply figure out whether this can safely be
> enabled at run time or not? If nobody uses PTP, then turn that on as
> soon as you get someone using PTP, turn this back off?

Yes that might work, and cause a performance improvement since TX processing
would be lighter. According to the R&D, the PTP problem can be overcome by
reading a special register that contains the timestamping, but I will need to
investigate that more.

I am going to start working on TSN features soon, and then I will get the "Drop
Status" topic again with the PTP issue solved and submit the patch again.

> 
> The problem is the commit message is extremely scarce about what this
> implies, and quite frankly, I don't understand what "application" means
> here, whether this means:
> 
> - application in the sense of the HW IP, which is then, the OS'
> networking stack presumably?
> 
> - application in the sense of the OS, which is sort of conflated with
> the socket user?
> 
> Worst case, can't you create a new netdev feature (e.g: similar to
> NETIF_F_RXFCS) that allows dynamically turning this on based on user
> configuration? Not suggesting this is the right approach here, but it
> should be thought of. Maybe an ethtool private flag is more appropriate?
> 
> One perspective you need to look at is the following:
> 
> - as an IP vendor, you would like to exercise every little configuration
> knob that the IP can be synthesized with, and that's fine and fun to do
> as long as you are in your controlled environment, this may result in
> custom DT properties, but those may not be candidate for the upstream
> kernel (just downstream)

Totally agree :), I done that with a local gitlab, since the Drop Status feature
will useful in some test cases I will be working on.
Sometimes we don't have the 100% of the thing and as you say some things work
well in a controlled environment but can be complicated in production
environment if not done to wok out of the box.

> 
> - customers want something that works out of the box, don't require
> reading a DT binding (in fact, not reading at all is best) and just get
> the promised performance, and the upstream kernel is the best place to
> get this done, but it needs to be painless

Understand and totally agree with you.

> 
> Finally, and completely tangential to this patch, pretty much *every*
> SoC that has a STMMAC is reasonably cheap to buy, so if Synopsys does
> not, you could, with your own personal money acquire a full set of
> Allwinner, Meson, Sunxi and what not platforms and put these in a test
> labs, heck, I'm sure someone could offer these. Most maintainers just
> collect HW as well as tribal knowledge about what typically breaks on
> this or that platform, and then run their tests on said platforms.
> 

I managed to get a several prototypes for ETH 3.x, 4.x and latest 5.x to have
more test power, but they are prototypes with the "ideal" design configuration,
so we can have suprises in real world hardware, since the original designs are
then adapted to fit the SoC. I totally understand this topic and I already
raised the question inside, so lets see how it develops.

> Hope this helps!
> 

Thank you very much for your e-mail, it was very helpful!

Joao