Synopsys Ethernet QoS

Synopsys Ethernet QoS

[Joao Pinto]

Dear David Miller,

These past 2 weeks we have been discussing the right way to go in terms of
Synopsys QoS support in the kernel.

The approach that raised more supporters was:

a) Test /stmicro/stmmac driver in a reference hardware prototyping platform (QoS
IPK) [Status: In Progress | 90% finished]

b) Merge the necessary features from AXIS’ synopsys based qos driver to the
/stmicro/stmmac
[Status: In Queue]

c) Rename /stmicro/stmmac driver to synopsys/ and re-factor the driver if necessary
[Status: In Queue]

d) Add QoS features incrementally to the new synopsys/ driver
[Status: In Queue]

This approach has the green light from AXIS and STMicro maintainers (Lars and
Peppe).

I would like to know if you support this plan.

Best Regards,
Joao

Re: Synopsys Ethernet QoS

[David Miller]

From: Joao Pinto <Joao.Pinto@synopsys.com>
Date: Fri, 9 Dec 2016 11:29:02 +0000

> Dear David Miller,
 ...
> I would like to know if you support this plan.

This is not how this works.

You need to discuss and work out a plan with the other people
with a direct interest in the existing drivers and maintainence.

Not me.

Re: Synopsys Ethernet QoS

[Joao Pinto]

Hi David,

Of course, I started a general discussion about the subject and those were the
conclusions, but I would like to know if you as the subsystem maintainer also
support the approach or have any suggestion.

Thanks,
Joao

Às 3:33 PM de 12/9/2016, David Miller escreveu:
> From: Joao Pinto <Joao.Pinto@synopsys.com>
> Date: Fri, 9 Dec 2016 11:29:02 +0000
> 
>> Dear David Miller,
>  ...
>> I would like to know if you support this plan.
> 
> This is not how this works.
> 
> You need to discuss and work out a plan with the other people
> with a direct interest in the existing drivers and maintainence.
> 
> Not me.
> 

Re: Synopsys Ethernet QoS

[David Miller]

From: Joao Pinto <Joao.Pinto@synopsys.com>
Date: Fri, 9 Dec 2016 15:36:38 +0000

> Of course, I started a general discussion about the subject and
> those were the conclusions, but I would like to know if you as the
> subsystem maintainer also support the approach or have any
> suggestion.

Generally, I support whatever the interested parties agree to.

But one thing I am against is changing the driver name for existing
users.  If an existing chip is supported by the stmmac driver for
existing users, they should still continue to use the "stmmac" driver.

Therefore, if consolidation changes the driver module name for
existing users, then that is not a good plan at all.

Re: Synopsys Ethernet QoS

[Joao Pinto]

Às 3:41 PM de 12/9/2016, David Miller escreveu:
> From: Joao Pinto <Joao.Pinto@synopsys.com>
> Date: Fri, 9 Dec 2016 15:36:38 +0000
> 
>> Of course, I started a general discussion about the subject and
>> those were the conclusions, but I would like to know if you as the
>> subsystem maintainer also support the approach or have any
>> suggestion.
> 
> Generally, I support whatever the interested parties agree to.
> 
> But one thing I am against is changing the driver name for existing
> users.  If an existing chip is supported by the stmmac driver for
> existing users, they should still continue to use the "stmmac" driver.
> 
> Therefore, if consolidation changes the driver module name for
> existing users, then that is not a good plan at all.
> 

Of course, 100% with you! Retro-compatibility for existing drivers is a must
have. The consolidation is going to be done with extreme careful.

Joao

Re: Synopsys Ethernet QoS

[Andy Shevchenko]

On Fri, Dec 9, 2016 at 5:41 PM, David Miller <davem@davemloft.net> wrote:

> But one thing I am against is changing the driver name for existing
> users.  If an existing chip is supported by the stmmac driver for
> existing users, they should still continue to use the "stmmac" driver.
>
> Therefore, if consolidation changes the driver module name for
> existing users, then that is not a good plan at all.

You have at least one supporter here. Though I jumped in to the
discussion very late, not sure if everyone have time to answer to
that.

-- 
With Best Regards,
Andy Shevchenko

Re: Synopsys Ethernet QoS

[Florian Fainelli]

On 12/09/2016 02:25 PM, Andy Shevchenko wrote:
> On Fri, Dec 9, 2016 at 5:41 PM, David Miller <davem@davemloft.net> wrote:
> 
>> But one thing I am against is changing the driver name for existing
>> users.  If an existing chip is supported by the stmmac driver for
>> existing users, they should still continue to use the "stmmac" driver.
>>
>> Therefore, if consolidation changes the driver module name for
>> existing users, then that is not a good plan at all.
> 
> You have at least one supporter here. Though I jumped in to the
> discussion very late, not sure if everyone have time to answer to
> that.

I don't have many stakes in the stmmac driver (or other Synopsys drivers
for that matter), but renaming seems like a terrible idea that is going
to make backporting of fixes difficult for distribution.

While moving the driver into a separate directory could be done, and git
knows how to track files, renaming the driver entirely would break many
platforms (including but not limited to, Device Tree) that you may not
have visibility over (compatible strings, properties, and platform
device driver name for instance).

It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe) did
actually pioneer the upstreaming effort, but it is good to see people
from Synopsys willing to fix that in the future.
-- 
Florian

Re: Synopsys Ethernet QoS

[Andy Shevchenko]

On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:

> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)

> did
> actually pioneer the upstreaming effort, but it is good to see people
> from Synopsys willing to fix that in the future.

Wait, you would like to tell that we have more than 2 drivers for the
same (okay, same vendor) IP?!
It's better to unify them earlier, than have n+ copies.

P.S. Though, I don't see how sxgbe got in the list. First glance on
the code doesn't show similarities.

-- 
With Best Regards,
Andy Shevchenko

Re: Synopsys Ethernet QoS

[Florian Fainelli]

Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
> 
>> did
>> actually pioneer the upstreaming effort, but it is good to see people
>> from Synopsys willing to fix that in the future.
> 
> Wait, you would like to tell that we have more than 2 drivers for the
> same (okay, same vendor) IP?!
> It's better to unify them earlier, than have n+ copies.

Unfortunately that is the case, see this email:

https://www.mail-archive.com/netdev@vger.kernel.org/msg142796.html

dwc_eth_qos and stmmac have some overlap. There seems to be work
underway to unify these two to begin with.

> 
> P.S. Though, I don't see how sxgbe got in the list. First glance on
> the code doesn't show similarities.

Well samsung/sxgbe looks potentially similar to amd/xgbe, but that's
just my cursory look at the code, it may very well be something entirely
different. The descriptor formats just look suspiciously similar.
-- 
Florian

Re: Synopsys Ethernet QoS

[Jie Deng]

On 2016/12/10 8:16, Andy Shevchenko wrote:
> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
>> did
>> actually pioneer the upstreaming effort, but it is good to see people
>> from Synopsys willing to fix that in the future.
> Wait, you would like to tell that we have more than 2 drivers for the
> same (okay, same vendor) IP?!
> It's better to unify them earlier, than have n+ copies.
>
> P.S. Though, I don't see how sxgbe got in the list. First glance on
> the code doesn't show similarities.
Glance on sxgbe_reg.h the register seems from Synopsys XGMAC IP... Probably,
amd-xgbe and sxgbe targeted the same IP

Re: Synopsys Ethernet QoS

[Joao Pinto]

Hi,

Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
>>
>>> did
>>> actually pioneer the upstreaming effort, but it is good to see people
>>> from Synopsys willing to fix that in the future.
>>
>> Wait, you would like to tell that we have more than 2 drivers for the
>> same (okay, same vendor) IP?!
>> It's better to unify them earlier, than have n+ copies.
> 
> Unfortunately that is the case, see this email:
> 
> https://www.mail-archive.com/netdev@vger.kernel.org/msg142796.html
> 
> dwc_eth_qos and stmmac have some overlap. There seems to be work
> underway to unify these two to begin with.
> 
>>
>> P.S. Though, I don't see how sxgbe got in the list. First glance on
>> the code doesn't show similarities.
> 
> Well samsung/sxgbe looks potentially similar to amd/xgbe, but that's
> just my cursory look at the code, it may very well be something entirely
> different. The descriptor formats just look suspiciously similar.
> 

Thank you for your inputs! Renaming seems to be a hotspot. I agree that maybe
instead of renaming (breaking retro-compatibility as David and Florian
mentioned), the best is to move stmmac to synopsys/ after merging *qos* and
removing it. As Florian mentioned, git is capable of detecting folder restructured.

@Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
driver would it be possible for you to make an initial analysis of what has to
be merged into Stmmac? This way the development would speed-up.

Thanks to all.

Joao

Re: Synopsys Ethernet QoS

[Giuseppe CAVALLARO]

Hello All.

On 12/12/2016 11:19 AM, Joao Pinto wrote:
> Hi,
>
> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>
>>>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
>>>
>>>> did
>>>> actually pioneer the upstreaming effort, but it is good to see people
>>>> from Synopsys willing to fix that in the future.
>>>
>>> Wait, you would like to tell that we have more than 2 drivers for the
>>> same (okay, same vendor) IP?!
>>> It's better to unify them earlier, than have n+ copies.
>>
>> Unfortunately that is the case, see this email:
>>
>> https://www.mail-archive.com/netdev@vger.kernel.org/msg142796.html
>>
>> dwc_eth_qos and stmmac have some overlap. There seems to be work
>> underway to unify these two to begin with.
>>
>>>
>>> P.S. Though, I don't see how sxgbe got in the list. First glance on
>>> the code doesn't show similarities.
>>
>> Well samsung/sxgbe looks potentially similar to amd/xgbe, but that's
>> just my cursory look at the code, it may very well be something entirely
>> different. The descriptor formats just look suspiciously similar.
>>
>
> Thank you for your inputs! Renaming seems to be a hotspot. I agree that maybe
> instead of renaming (breaking retro-compatibility as David and Florian
> mentioned), the best is to move stmmac to synopsys/ after merging *qos* and
> removing it. As Florian mentioned, git is capable of detecting folder restructured.
>
> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
> driver would it be possible for you to make an initial analysis of what has to
> be merged into Stmmac? This way the development would speed-up.

my priority on this topic was to help Joao to easily port his
platform on stmmac and IIUC we already achieved some results.
For several reasons, we had discussed to support GMAC IP with stmmac
and, as first stage, to evaluate and port the relevant changes
from  dwc_eth_qos.c. If renaming of moving the stmmac should be
discussed later and for sure all together could find the best solution
in the right moment where some critical patches and supports are in
place.

As highlighted by Jie, the XLGMAC is another IP (that I do not know
at all so I cannot say if there is some way to support it
by using stmmac).

Thanks a lot.

peppe

>
> Thanks to all.
>
> Joao
>

Re: Re: Synopsys Ethernet QoS

[Niklas Cassel]

On 12/12/2016 11:19 AM, Joao Pinto wrote:
> Hi,
>
> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>
>>>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
>>>> did
>>>> actually pioneer the upstreaming effort, but it is good to see people
>>>> from Synopsys willing to fix that in the future.
>>> Wait, you would like to tell that we have more than 2 drivers for the
>>> same (okay, same vendor) IP?!
>>> It's better to unify them earlier, than have n+ copies.
>> Unfortunately that is the case, see this email:
>>
>> https://www.mail-archive.com/netdev@vger.kernel.org/msg142796.html
>>
>> dwc_eth_qos and stmmac have some overlap. There seems to be work
>> underway to unify these two to begin with.
>>
>>> P.S. Though, I don't see how sxgbe got in the list. First glance on
>>> the code doesn't show similarities.
>> Well samsung/sxgbe looks potentially similar to amd/xgbe, but that's
>> just my cursory look at the code, it may very well be something entirely
>> different. The descriptor formats just look suspiciously similar.
>>
> Thank you for your inputs! Renaming seems to be a hotspot. I agree that maybe
> instead of renaming (breaking retro-compatibility as David and Florian
> mentioned), the best is to move stmmac to synopsys/ after merging *qos* and
> removing it. As Florian mentioned, git is capable of detecting folder restructured.
>
> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
> driver would it be possible for you to make an initial analysis of what has to
> be merged into Stmmac? This way the development would speed-up.

I can answer that question.

I've sent out 12 patches to the stmmac driver
(all patches are included in the current net-next tree),
with these patches the stmmac driver works properly on Axis hardware
(we use Synopsys GMAC 4.10a synthesized with multiple TX queues).
stmmac's DT binding has also been extended with properties that
existed in DWC EQoS's DT binding, such as no-pbl-x8, txpbl, rxpbl.

Since we have no problem updating the DTB together with the kernel,
we will simply move to using the start using the stmmac driver,
with stmmac's DT binding.

However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
I don't how easy it would be for them to switch to stmmac's DT binding.
(Adding Stephen Warren to CC.)

The reset sequence that Lars Persson was worried about is not an issue
with the stmmac driver.




There are some performance problems with the stmmac driver though:

When running iperf3 with 3 streams:
iperf3 -c 192.168.0.90 -P 3 -t 30
iperf3 -c 192.168.0.90 -P 3 -t 30 -R

I get really bad fairness between the streams.

This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.
We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
and we don't see the same problem.

Also netperf TCP_RR and UDP_RR gives really bad results compared to the
dwceqos driver (without IRQ coalescing).
2000 transactions/sec vs 9000 transactions/sec.
Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
gives the same performance. I guess it's a trade off, low CPU usage
vs low latency, so I don't know how important TCP_RR/UDP_RR really is.

The best thing would be to get a good working TX IRQ coalesce
implementation with HR timers in stmmac.
Perhaps it should also be investigated if the RX interrupt watchdog
timeout should have a lower default value.



>
> Thanks to all.
>
> Joao

RE: Re: Synopsys Ethernet QoS

[Stephen Warren]

Niklas Cassel wrote at Monday, December 12, 2016 9:25 AM:
...
> However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
> I don't how easy it would be for them to switch to stmmac's DT binding.
> (Adding Stephen Warren to CC.)

I don't believe there's any issue switching drivers, so long as the new one
works on our HW. However, we can't switch DT binding since that's an ABI.
So, if we switch drivers, the "new" driver needs to support all existing
DT bindings.

FWIW, I'd recommend that we don't name the "new" driver stmmac or anything
like that. Rather, name it after the IP block so that any new user of the same
IP block will find the name they expect in the source tree, and just use it.
Renaming the "new" driver to dwc_eqos or similar might be one way to achieve
that.

-- 
nvpublic

Re: Synopsys Ethernet QoS

[Giuseppe CAVALLARO]

On 12/12/2016 5:25 PM, Niklas Cassel wrote:
>
>
> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>> Hi,
>>
>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>
>>>>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
>>>>> did
>>>>> actually pioneer the upstreaming effort, but it is good to see people
>>>>> from Synopsys willing to fix that in the future.
>>>> Wait, you would like to tell that we have more than 2 drivers for the
>>>> same (okay, same vendor) IP?!
>>>> It's better to unify them earlier, than have n+ copies.
>>> Unfortunately that is the case, see this email:
>>>
>>> https://www.mail-archive.com/netdev@vger.kernel.org/msg142796.html
>>>
>>> dwc_eth_qos and stmmac have some overlap. There seems to be work
>>> underway to unify these two to begin with.
>>>
>>>> P.S. Though, I don't see how sxgbe got in the list. First glance on
>>>> the code doesn't show similarities.
>>> Well samsung/sxgbe looks potentially similar to amd/xgbe, but that's
>>> just my cursory look at the code, it may very well be something entirely
>>> different. The descriptor formats just look suspiciously similar.
>>>
>> Thank you for your inputs! Renaming seems to be a hotspot. I agree that maybe
>> instead of renaming (breaking retro-compatibility as David and Florian
>> mentioned), the best is to move stmmac to synopsys/ after merging *qos* and
>> removing it. As Florian mentioned, git is capable of detecting folder restructured.
>>
>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>> driver would it be possible for you to make an initial analysis of what has to
>> be merged into Stmmac? This way the development would speed-up.
>
> I can answer that question.
>
> I've sent out 12 patches to the stmmac driver
> (all patches are included in the current net-next tree),

ok I have seen these patches applied, I had just a minor concern about
the  failure when DMA configuration is missing.
In these years, I have noticed that, for this kind of HW, default DMA
configuration is usually good to have a driver working. AHB, AXI
parameters can be provided to have a best tuning or to fix know issues
on some platforms. So IMO, we should relax the check with a warning.
Please, consider that, the stmmac also supports very old MAC10/100
versions where the DMA configuration was often never passed.

> with these patches the stmmac driver works properly on Axis hardware
> (we use Synopsys GMAC 4.10a synthesized with multiple TX queues).

perfect and thanks a lot for this effort.

> stmmac's DT binding has also been extended with properties that
> existed in DWC EQoS's DT binding, such as no-pbl-x8, txpbl, rxpbl.
>
> Since we have no problem updating the DTB together with the kernel,
> we will simply move to using the start using the stmmac driver,
> with stmmac's DT binding.
>
> However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
> I don't how easy it would be for them to switch to stmmac's DT binding.
> (Adding Stephen Warren to CC.)

ok

>
> The reset sequence that Lars Persson was worried about is not an issue
> with the stmmac driver.
>

thx for this check.

>
> There are some performance problems with the stmmac driver though:
>
> When running iperf3 with 3 streams:
> iperf3 -c 192.168.0.90 -P 3 -t 30
> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
>
> I get really bad fairness between the streams.

Can you confirm you are using the 4.xxa version?

This doesn't match with Alex's experiments on ARM platforms.

> This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
> Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.

this doesn't match with what we had seen but I am happy and open to
review and accept new strategy.

> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
> and we don't see the same problem.

please, if you have new patch add me on CC and we will review all
together.

> Also netperf TCP_RR and UDP_RR gives really bad results compared to the
> dwceqos driver (without IRQ coalescing).
> 2000 transactions/sec vs 9000 transactions/sec.
> Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
> gives the same performance. I guess it's a trade off, low CPU usage
> vs low latency, so I don't know how important TCP_RR/UDP_RR really is.
>
> The best thing would be to get a good working TX IRQ coalesce
> implementation with HR timers in stmmac.

as said, welcome patches.

Basically, the default tuning of coalescence parameters comes from
ST platform experiences. I mean, we tuned to driver to have good
performance and saving CPU on SH4 (UP) and ARM (SMP) systems.
In these years, these default was accepted but, if today we need
to change something welcome effort. On my side, I can try to
perform some bench to see if I have regressions on not.

> Perhaps it should also be investigated if the RX interrupt watchdog
> timeout should have a lower default value.

Do not expect to get many improvements to play with the HW watchdog
due to the poor granularity of the Receive Interrupt Watchdog Timer
Count.

Regards
Peppe

>
>
>
>>
>> Thanks to all.
>>
>> Joao
>
>

Re: Synopsys Ethernet QoS

[Niklas Cassel]

On 12/13/2016 08:22 AM, Giuseppe CAVALLARO wrote:
> On 12/12/2016 5:25 PM, Niklas Cassel wrote:
>>
>>
>> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>>> Hi,
>>>
>>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>
>>>>>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
>>>>>> did
>>>>>> actually pioneer the upstreaming effort, but it is good to see people
>>>>>> from Synopsys willing to fix that in the future.
>>>>> Wait, you would like to tell that we have more than 2 drivers for the
>>>>> same (okay, same vendor) IP?!
>>>>> It's better to unify them earlier, than have n+ copies.
>>>> Unfortunately that is the case, see this email:
>>>>
>>>> https://www.mail-archive.com/netdev@vger.kernel.org/msg142796.html
>>>>
>>>> dwc_eth_qos and stmmac have some overlap. There seems to be work
>>>> underway to unify these two to begin with.
>>>>
>>>>> P.S. Though, I don't see how sxgbe got in the list. First glance on
>>>>> the code doesn't show similarities.
>>>> Well samsung/sxgbe looks potentially similar to amd/xgbe, but that's
>>>> just my cursory look at the code, it may very well be something entirely
>>>> different. The descriptor formats just look suspiciously similar.
>>>>
>>> Thank you for your inputs! Renaming seems to be a hotspot. I agree that maybe
>>> instead of renaming (breaking retro-compatibility as David and Florian
>>> mentioned), the best is to move stmmac to synopsys/ after merging *qos* and
>>> removing it. As Florian mentioned, git is capable of detecting folder restructured.
>>>
>>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>>> driver would it be possible for you to make an initial analysis of what has to
>>> be merged into Stmmac? This way the development would speed-up.
>>
>> I can answer that question.
>>
>> I've sent out 12 patches to the stmmac driver
>> (all patches are included in the current net-next tree),
>
> ok I have seen these patches applied, I had just a minor concern about
> the  failure when DMA configuration is missing.
> In these years, I have noticed that, for this kind of HW, default DMA
> configuration is usually good to have a driver working. AHB, AXI
> parameters can be provided to have a best tuning or to fix know issues
> on some platforms. So IMO, we should relax the check with a warning.
> Please, consider that, the stmmac also supports very old MAC10/100
> versions where the DMA configuration was often never passed.
>

This might be a bit off-topic, but:
The patch should not affect any existing code.
All glue drivers uses stmmac_probe_config_dt,
which sets a default value if the property is missing or zero.
The PCI glue driver sets the property explicitly.
Hence, all current code should not be affected.
The error check was added as a sanity check, just in case some
new code is added, which bypasses stmmac_probe_config_dt,
lets say support for GMAC4 in the PCI glue driver.


>
>>
>> There are some performance problems with the stmmac driver though:
>>
>> When running iperf3 with 3 streams:
>> iperf3 -c 192.168.0.90 -P 3 -t 30
>> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
>>
>> I get really bad fairness between the streams.
>
> Can you confirm you are using the 4.xxa version?

Yes, 4.10a.
Reading the MAC_Version register gives:
0x00004041
Where SNPSVER is bit 7:0, so 0x41.


>
> This doesn't match with Alex's experiments on ARM platforms.
>

We are also using an ARM platform.
There is no problem with throughput.
The problem is fairness between the streams,
when using for example 3 streams in iperf3.
Did Alex test this? I could not find Alex mail in the netdev archives,
could you link to it?
The problem goes away when disabling TX IRQ coalescing,
which I guess makes sense, since like David Miller said:

"the driver is using non-highres timers
to implement the TX coalescing.  [...]
1 HZ, which is the lowest granularity of non-highres timers in the
kernel, is variable as well as already too large of a delay for
effective TX coalescing."

We are using CONFIG_HZ=100, not CONFIG_HZ=1000.

So if there is a long time before handling interrupts,
I guess that it makes sense that one stream could
get an advantage in the net scheduler.

If I find the time, and if no one beats me to it, I will try to replace
the normal timers with HR timers + a smaller default timeout.


>
>> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
>> and we don't see the same problem.
>
> please, if you have new patch add me on CC and we will review all
> together.

I think you misunderstood me, we have a local patch for the synopsys/dwc_eth_qos.c
version, not for stmmac.

I have been using get_maintainer.pl, so you should have been added to all my patches,
but if I send any further patches, I will make sure that you are not excluded.

Re: Synopsys Ethernet QoS

[Giuseppe CAVALLARO]

Hello Niklas

On 12/13/2016 10:38 AM, Niklas Cassel wrote:
> On 12/13/2016 08:22 AM, Giuseppe CAVALLARO wrote:
>> On 12/12/2016 5:25 PM, Niklas Cassel wrote:
>>>
>>>
>>> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>>>> Hi,
>>>>
>>>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>>
>>>>>>> It's kind of sad that customers of that IP (stmmac, amd-xgbe, sxgbe)
>>>>>>> did
>>>>>>> actually pioneer the upstreaming effort, but it is good to see people
>>>>>>> from Synopsys willing to fix that in the future.
>>>>>> Wait, you would like to tell that we have more than 2 drivers for the
>>>>>> same (okay, same vendor) IP?!
>>>>>> It's better to unify them earlier, than have n+ copies.
>>>>> Unfortunately that is the case, see this email:
>>>>>
>>>>> https://www.mail-archive.com/netdev@vger.kernel.org/msg142796.html
>>>>>
>>>>> dwc_eth_qos and stmmac have some overlap. There seems to be work
>>>>> underway to unify these two to begin with.
>>>>>
>>>>>> P.S. Though, I don't see how sxgbe got in the list. First glance on
>>>>>> the code doesn't show similarities.
>>>>> Well samsung/sxgbe looks potentially similar to amd/xgbe, but that's
>>>>> just my cursory look at the code, it may very well be something entirely
>>>>> different. The descriptor formats just look suspiciously similar.
>>>>>
>>>> Thank you for your inputs! Renaming seems to be a hotspot. I agree that maybe
>>>> instead of renaming (breaking retro-compatibility as David and Florian
>>>> mentioned), the best is to move stmmac to synopsys/ after merging *qos* and
>>>> removing it. As Florian mentioned, git is capable of detecting folder restructured.
>>>>
>>>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>>>> driver would it be possible for you to make an initial analysis of what has to
>>>> be merged into Stmmac? This way the development would speed-up.
>>>
>>> I can answer that question.
>>>
>>> I've sent out 12 patches to the stmmac driver
>>> (all patches are included in the current net-next tree),
>>
>> ok I have seen these patches applied, I had just a minor concern about
>> the  failure when DMA configuration is missing.
>> In these years, I have noticed that, for this kind of HW, default DMA
>> configuration is usually good to have a driver working. AHB, AXI
>> parameters can be provided to have a best tuning or to fix know issues
>> on some platforms. So IMO, we should relax the check with a warning.
>> Please, consider that, the stmmac also supports very old MAC10/100
>> versions where the DMA configuration was often never passed.
>>
>
> This might be a bit off-topic, but:

yes indeed ;-)

> The patch should not affect any existing code.
> All glue drivers uses stmmac_probe_config_dt,
> which sets a default value if the property is missing or zero.
> The PCI glue driver sets the property explicitly.
> Hence, all current code should not be affected.
> The error check was added as a sanity check, just in case some
> new code is added, which bypasses stmmac_probe_config_dt,
> lets say support for GMAC4 in the PCI glue driver.
>

ok

>>
>>>
>>> There are some performance problems with the stmmac driver though:
>>>
>>> When running iperf3 with 3 streams:
>>> iperf3 -c 192.168.0.90 -P 3 -t 30
>>> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
>>>
>>> I get really bad fairness between the streams.
>>
>> Can you confirm you are using the 4.xxa version?
>
> Yes, 4.10a.
> Reading the MAC_Version register gives:
> 0x00004041
> Where SNPSVER is bit 7:0, so 0x41.

ok

>
>
>>
>> This doesn't match with Alex's experiments on ARM platforms.
>>
>
> We are also using an ARM platform.
> There is no problem with throughput.

ok

> The problem is fairness between the streams,
> when using for example 3 streams in iperf3.

hmm I let Alex to reply.

AFAIK, other people played with stream tests and
no issue but, if you get problems so we have to
investigate.

> Did Alex test this? I could not find Alex mail in the netdev archives,
> could you link to it?
> The problem goes away when disabling TX IRQ coalescing,
> which I guess makes sense, since like David Miller said:
>
> "the driver is using non-highres timers
> to implement the TX coalescing.  [...]
> 1 HZ, which is the lowest granularity of non-highres timers in the
> kernel, is variable as well as already too large of a delay for
> effective TX coalescing."
>
> We are using CONFIG_HZ=100, not CONFIG_HZ=1000.

1000 was a default on our platforms

>
> So if there is a long time before handling interrupts,
> I guess that it makes sense that one stream could
> get an advantage in the net scheduler.

ok

>
> If I find the time, and if no one beats me to it, I will try to replace
> the normal timers with HR timers + a smaller default timeout.

that's great.

>>
>>> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
>>> and we don't see the same problem.
>>
>> please, if you have new patch add me on CC and we will review all
>> together.
>
> I think you misunderstood me, we have a local patch for the synopsys/dwc_eth_qos.c
> version, not for stmmac.

oops, sorry

>
> I have been using get_maintainer.pl, so you should have been added to all my patches,
> but if I send any further patches, I will make sure that you are not excluded.

thanks a lot

Regards
Peppe

>
>

Re: Synopsys Ethernet QoS

[Joao Pinto]

Hi Niklas,

Às 4:25 PM de 12/12/2016, Niklas Cassel escreveu:
> 
> 
> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>> Hi,
>>
>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:

(snip...)


>>
>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>> driver would it be possible for you to make an initial analysis of what has to
>> be merged into Stmmac? This way the development would speed-up.
> 
> I can answer that question.
> 
> I've sent out 12 patches to the stmmac driver
> (all patches are included in the current net-next tree),
> with these patches the stmmac driver works properly on Axis hardware
> (we use Synopsys GMAC 4.10a synthesized with multiple TX queues).
> stmmac's DT binding has also been extended with properties that
> existed in DWC EQoS's DT binding, such as no-pbl-x8, txpbl, rxpbl.
> 
> Since we have no problem updating the DTB together with the kernel,
> we will simply move to using the start using the stmmac driver,
> with stmmac's DT binding.
> 
> However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
> I don't how easy it would be for them to switch to stmmac's DT binding.
> (Adding Stephen Warren to CC.)
> 
> The reset sequence that Lars Persson was worried about is not an issue
> with the stmmac driver.

Great! So you saying that stmmac works great with AXIS hardware and no need to
merge specific code from AXIS' *qos* driver?

> 
> 
> 
> 
> There are some performance problems with the stmmac driver though:
> 
> When running iperf3 with 3 streams:
> iperf3 -c 192.168.0.90 -P 3 -t 30
> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
> 
> I get really bad fairness between the streams.
> 
> This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
> Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.
> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
> and we don't see the same problem.
> 
> Also netperf TCP_RR and UDP_RR gives really bad results compared to the
> dwceqos driver (without IRQ coalescing).
> 2000 transactions/sec vs 9000 transactions/sec.
> Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
> gives the same performance. I guess it's a trade off, low CPU usage
> vs low latency, so I don't know how important TCP_RR/UDP_RR really is.
> 
> The best thing would be to get a good working TX IRQ coalesce
> implementation with HR timers in stmmac.
> Perhaps it should also be investigated if the RX interrupt watchdog
> timeout should have a lower default value.
> 
> 
> 
>>
>> Thanks to all.
>>
>> Joao
> 

Re: Synopsys Ethernet QoS

[Niklas Cassel]

On 12/13/2016 12:49 PM, Joao Pinto wrote:
> Hi Niklas,
>
> Às 4:25 PM de 12/12/2016, Niklas Cassel escreveu:
>>
>> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>>> Hi,
>>>
>>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> (snip...)
>
>
>>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>>> driver would it be possible for you to make an initial analysis of what has to
>>> be merged into Stmmac? This way the development would speed-up.
>> I can answer that question.
>>
>> I've sent out 12 patches to the stmmac driver
>> (all patches are included in the current net-next tree),
>> with these patches the stmmac driver works properly on Axis hardware
>> (we use Synopsys GMAC 4.10a synthesized with multiple TX queues).
>> stmmac's DT binding has also been extended with properties that
>> existed in DWC EQoS's DT binding, such as no-pbl-x8, txpbl, rxpbl.
>>
>> Since we have no problem updating the DTB together with the kernel,
>> we will simply move to using the start using the stmmac driver,
>> with stmmac's DT binding.
>>
>> However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
>> I don't how easy it would be for them to switch to stmmac's DT binding.
>> (Adding Stephen Warren to CC.)
>>
>> The reset sequence that Lars Persson was worried about is not an issue
>> with the stmmac driver.
> Great! So you saying that stmmac works great with AXIS hardware and no need to
> merge specific code from AXIS' *qos* driver?

Yes. From Axis point of view, we are done.
stmmac works and we will move to that driver + DT binding.

However, it seems like Stephen Warren will NAK if you try to remove
synopsys/dwc_eth_qos.c before
Documentation/devicetree/bindings/net/stmmac.txt
is compatible with
Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt

You might want to sync with him. I have no idea, but perhaps they are
only using a subset of all the available properties. Perhaps,
only implementing what they are using might be enough, I don't know.
I couldn't find their DTS in arch/arm/dts.
I guess you might want to know David Miller's opinion,
since he's the one who decides in the end.

>>
>>
>>
>> There are some performance problems with the stmmac driver though:
>>
>> When running iperf3 with 3 streams:
>> iperf3 -c 192.168.0.90 -P 3 -t 30
>> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
>>
>> I get really bad fairness between the streams.
>>
>> This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
>> Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.
>> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
>> and we don't see the same problem.
>>
>> Also netperf TCP_RR and UDP_RR gives really bad results compared to the
>> dwceqos driver (without IRQ coalescing).
>> 2000 transactions/sec vs 9000 transactions/sec.
>> Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
>> gives the same performance. I guess it's a trade off, low CPU usage
>> vs low latency, so I don't know how important TCP_RR/UDP_RR really is.
>>
>> The best thing would be to get a good working TX IRQ coalesce
>> implementation with HR timers in stmmac.
>> Perhaps it should also be investigated if the RX interrupt watchdog
>> timeout should have a lower default value.
>>
>>
>>
>>> Thanks to all.
>>>
>>> Joao

Re: Synopsys Ethernet QoS

[Lars Persson]

> 13 dec. 2016 kl. 13:31 skrev Niklas Cassel <Niklas.Cassel@axis.com>:
> 
> 
> 
>> On 12/13/2016 12:49 PM, Joao Pinto wrote:
>> Hi Niklas,
>> 
>> Às 4:25 PM de 12/12/2016, Niklas Cassel escreveu:
>>> 
>>>> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>>>> Hi,
>>>> 
>>>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>>>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> (snip...)
>> 
>> 
>>>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>>>> driver would it be possible for you to make an initial analysis of what has to
>>>> be merged into Stmmac? This way the development would speed-up.
>>> I can answer that question.
>>> 
>>> I've sent out 12 patches to the stmmac driver
>>> (all patches are included in the current net-next tree),
>>> with these patches the stmmac driver works properly on Axis hardware
>>> (we use Synopsys GMAC 4.10a synthesized with multiple TX queues).
>>> stmmac's DT binding has also been extended with properties that
>>> existed in DWC EQoS's DT binding, such as no-pbl-x8, txpbl, rxpbl.
>>> 
>>> Since we have no problem updating the DTB together with the kernel,
>>> we will simply move to using the start using the stmmac driver,
>>> with stmmac's DT binding.
>>> 
>>> However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
>>> I don't how easy it would be for them to switch to stmmac's DT binding.
>>> (Adding Stephen Warren to CC.)
>>> 
>>> The reset sequence that Lars Persson was worried about is not an issue
>>> with the stmmac driver.
>> Great! So you saying that stmmac works great with AXIS hardware and no need to
>> merge specific code from AXIS' *qos* driver?
> 
> Yes. From Axis point of view, we are done.
> stmmac works and we will move to that driver + DT binding.
> 
> However, it seems like Stephen Warren will NAK if you try to remove
> synopsys/dwc_eth_qos.c before
> Documentation/devicetree/bindings/net/stmmac.txt
> is compatible with
> Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
> 
> You might want to sync with him. I have no idea, but perhaps they are
> only using a subset of all the available properties. Perhaps,
> only implementing what they are using might be enough, I don't know.
> I couldn't find their DTS in arch/arm/dts.
> I guess you might want to know David Miller's opinion,
> since he's the one who decides in the end.

I will also NACK removal of dwc_eth_qos.c until the binding is implemented elsewhere.


> 
>>> 
>>> 
>>> 
>>> There are some performance problems with the stmmac driver though:
>>> 
>>> When running iperf3 with 3 streams:
>>> iperf3 -c 192.168.0.90 -P 3 -t 30
>>> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
>>> 
>>> I get really bad fairness between the streams.
>>> 
>>> This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
>>> Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.
>>> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
>>> and we don't see the same problem.
>>> 
>>> Also netperf TCP_RR and UDP_RR gives really bad results compared to the
>>> dwceqos driver (without IRQ coalescing).
>>> 2000 transactions/sec vs 9000 transactions/sec.
>>> Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
>>> gives the same performance. I guess it's a trade off, low CPU usage
>>> vs low latency, so I don't know how important TCP_RR/UDP_RR really is.
>>> 
>>> The best thing would be to get a good working TX IRQ coalesce
>>> implementation with HR timers in stmmac.
>>> Perhaps it should also be investigated if the RX interrupt watchdog
>>> timeout should have a lower default value.
>>> 
>>> 
>>> 
>>>> Thanks to all.
>>>> 
>>>> Joao
> 

Re: Synopsys Ethernet QoS

[Joao Pinto]

Às 12:50 PM de 12/13/2016, Lars Persson escreveu:
> 
>> 13 dec. 2016 kl. 13:31 skrev Niklas Cassel <Niklas.Cassel@axis.com>:
>>
>>
>>
>>> On 12/13/2016 12:49 PM, Joao Pinto wrote:
>>> Hi Niklas,
>>>
>>> Às 4:25 PM de 12/12/2016, Niklas Cassel escreveu:
>>>>
>>>>> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>>>>> Hi,
>>>>>
>>>>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>>>>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>> (snip...)
>>>
>>>
>>>>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>>>>> driver would it be possible for you to make an initial analysis of what has to
>>>>> be merged into Stmmac? This way the development would speed-up.
>>>> I can answer that question.
>>>>
>>>> I've sent out 12 patches to the stmmac driver
>>>> (all patches are included in the current net-next tree),
>>>> with these patches the stmmac driver works properly on Axis hardware
>>>> (we use Synopsys GMAC 4.10a synthesized with multiple TX queues).
>>>> stmmac's DT binding has also been extended with properties that
>>>> existed in DWC EQoS's DT binding, such as no-pbl-x8, txpbl, rxpbl.
>>>>
>>>> Since we have no problem updating the DTB together with the kernel,
>>>> we will simply move to using the start using the stmmac driver,
>>>> with stmmac's DT binding.
>>>>
>>>> However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
>>>> I don't how easy it would be for them to switch to stmmac's DT binding.
>>>> (Adding Stephen Warren to CC.)
>>>>
>>>> The reset sequence that Lars Persson was worried about is not an issue
>>>> with the stmmac driver.
>>> Great! So you saying that stmmac works great with AXIS hardware and no need to
>>> merge specific code from AXIS' *qos* driver?
>>
>> Yes. From Axis point of view, we are done.
>> stmmac works and we will move to that driver + DT binding.
>>
>> However, it seems like Stephen Warren will NAK if you try to remove
>> synopsys/dwc_eth_qos.c before
>> Documentation/devicetree/bindings/net/stmmac.txt
>> is compatible with
>> Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
>>
>> You might want to sync with him. I have no idea, but perhaps they are
>> only using a subset of all the available properties. Perhaps,
>> only implementing what they are using might be enough, I don't know.
>> I couldn't find their DTS in arch/arm/dts.
>> I guess you might want to know David Miller's opinion,
>> since he's the one who decides in the end.
> 
> I will also NACK removal of dwc_eth_qos.c until the binding is implemented elsewhere.

Totally agree.
@Niklas: David Miller is informed of what we are planning to do. Can you
estimate the effort of merging the axis driver device tree bindings? If there
was anyone from axis to do the merge would be better since you are familiar with
it. What do you think?

> 
> 
>>
>>>>
>>>>
>>>>
>>>> There are some performance problems with the stmmac driver though:
>>>>
>>>> When running iperf3 with 3 streams:
>>>> iperf3 -c 192.168.0.90 -P 3 -t 30
>>>> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
>>>>
>>>> I get really bad fairness between the streams.
>>>>
>>>> This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
>>>> Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.
>>>> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
>>>> and we don't see the same problem.
>>>>
>>>> Also netperf TCP_RR and UDP_RR gives really bad results compared to the
>>>> dwceqos driver (without IRQ coalescing).
>>>> 2000 transactions/sec vs 9000 transactions/sec.
>>>> Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
>>>> gives the same performance. I guess it's a trade off, low CPU usage
>>>> vs low latency, so I don't know how important TCP_RR/UDP_RR really is.
>>>>
>>>> The best thing would be to get a good working TX IRQ coalesce
>>>> implementation with HR timers in stmmac.
>>>> Perhaps it should also be investigated if the RX interrupt watchdog
>>>> timeout should have a lower default value.
>>>>
>>>>
>>>>
>>>>> Thanks to all.
>>>>>
>>>>> Joao
>>

Re: Re: Synopsys Ethernet QoS

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

Hi!

> There are some performance problems with the stmmac driver though:
> 
> When running iperf3 with 3 streams:
> iperf3 -c 192.168.0.90 -P 3 -t 30
> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
> 
> I get really bad fairness between the streams.
> 
> This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
> Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.

Yes, I'm hitting the same problem

https://lkml.org/lkml/2016/12/11/90

> Also netperf TCP_RR and UDP_RR gives really bad results compared to the
> dwceqos driver (without IRQ coalescing).
> 2000 transactions/sec vs 9000 transactions/sec.
> Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
> gives the same performance. I guess it's a trade off, low CPU usage
> vs low latency, so I don't know how important TCP_RR/UDP_RR really is.

Well... 75% performance hit is a bug, plain and simple, not CPU tradeoff.

> The best thing would be to get a good working TX IRQ coalesce
> implementation with HR timers in stmmac.

Actually it seems that using HR timers is not the only problem, AFAICT
the logic is wrong way around. (But yes, it needs to be HR timer, too,
and probably packet count needs to be reduced, too.)

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Synopsys Ethernet QoS

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

Hi!

> So if there is a long time before handling interrupts,
> I guess that it makes sense that one stream could
> get an advantage in the net scheduler.
> 
> If I find the time, and if no one beats me to it, I will try to replace
> the normal timers with HR timers + a smaller default timeout.
> 

Can you try something like this? Highres timers will be needed, too,
but this fixes the logic problem.

You'll need to apply it twice as code is copy&pasted.

Best regards,
									Pavel

+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

 	 */
 	priv->tx_count_frames += nfrags + 1;
 	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
-		mod_timer(&priv->txtimer,
-			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
+		if (priv->tx_count_frames == nfrags + 1)
+			mod_timer(&priv->txtimer,
+				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
 	} else {
 		priv->tx_count_frames = 0;
 		priv->hw->desc->set_tx_ic(desc);


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Synopsys Ethernet QoS

[Joao Pinto]

Hi,

Às 12:57 PM de 12/14/2016, Pavel Machek escreveu:
> Hi!
> 
>> So if there is a long time before handling interrupts,
>> I guess that it makes sense that one stream could
>> get an advantage in the net scheduler.
>>
>> If I find the time, and if no one beats me to it, I will try to replace
>> the normal timers with HR timers + a smaller default timeout.
>>
> 
> Can you try something like this? Highres timers will be needed, too,
> but this fixes the logic problem.
> 
> You'll need to apply it twice as code is copy&pasted.
> 
> Best regards,
> 									Pavel
> 
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
>  	 */
>  	priv->tx_count_frames += nfrags + 1;
>  	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +		if (priv->tx_count_frames == nfrags + 1)
> +			mod_timer(&priv->txtimer,
> +				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>  	} else {
>  		priv->tx_count_frames = 0;
>  		priv->hw->desc->set_tx_ic(desc);
> 
> 

I know that this is completely of topic, but I am facing a dificulty with
stmmac. I have interrupts, mac well configured rx packets being received
successfully, but TX is not working, resulting in Tx errors = Total TX packets.
I have made a lot of debug and my conclusions is that by some reason when using
stmmac after starting tx dma, the hw state machine enters a deadend state
resulting in those errors. Anyone faced this trouble?

Thanks.

stmmac: lockups (was Re: Synopsys Ethernet QoS)

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

Hi!

> I know that this is completely of topic, but I am facing a dificulty with
> stmmac. I have interrupts, mac well configured rx packets being received
> successfully, but TX is not working, resulting in Tx errors = Total TX packets.
> I have made a lot of debug and my conclusions is that by some reason when using
> stmmac after starting tx dma, the hw state machine enters a deadend state
> resulting in those errors. Anyone faced this trouble?

Well.... what I'm debugging are lockups after many packets transmitted
(followed by netdev watchdog; stmmac_tx_err() does not work for me, it
kills the device even when run from working state; ifconfig down/up
helps). 4.4 locks up in minutes to hours, 4.9 seems to work better
(but I believe I seen a lockup there, too; once).

So... probably different problem?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Synopsys Ethernet QoS

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

Hi!

> I know that this is completely of topic, but I am facing a dificulty with
> stmmac. I have interrupts, mac well configured rx packets being received
> successfully, but TX is not working, resulting in Tx errors = Total TX packets.
> I have made a lot of debug and my conclusions is that by some reason when using
> stmmac after starting tx dma, the hw state machine enters a deadend state
> resulting in those errors. Anyone faced this trouble?

Actually, I see you have address @synopsys.com, would you have
documentation for the chip?

I'm trying to understand stmmac_tx_clean() and docs would help...

Thanks,
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Synopsys Ethernet QoS

[Niklas Cassel]

On 12/14/2016 01:57 PM, Pavel Machek wrote:
> Hi!
>
>> So if there is a long time before handling interrupts,
>> I guess that it makes sense that one stream could
>> get an advantage in the net scheduler.
>>
>> If I find the time, and if no one beats me to it, I will try to replace
>> the normal timers with HR timers + a smaller default timeout.
>>
> Can you try something like this? Highres timers will be needed, too,
> but this fixes the logic problem.

Hello Pavel

I tried your patch, but unfortunately I get a tx queue timeout.
After that, I cannot ping.

[   22.075782] ------------[ cut here ]------------
[   22.080430] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:316 dev_watchdog+0x240/0x258
[   22.088704] NETDEV WATCHDOG: eth0 (stmmaceth): transmit queue 0 timed out
[   22.095491] Modules linked in:
[   22.098552] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.9.0-axis3-devel #126
[   22.105592] Hardware name: Axis ARTPEC-6 Platform
[   22.110301] [<80110568>] (unwind_backtrace) from [<8010c2bc>] (show_stack+0x18/0x1c)
[   22.118043] [<8010c2bc>] (show_stack) from [<80433544>] (dump_stack+0x80/0xa0)
[   22.125264] [<80433544>] (dump_stack) from [<8011f9f0>] (__warn+0xe0/0x10c)
[   22.132221] [<8011f9f0>] (__warn) from [<8011fadc>] (warn_slowpath_fmt+0x40/0x50)
[   22.139700] [<8011fadc>] (warn_slowpath_fmt) from [<805e626c>] (dev_watchdog+0x240/0x258)
[   22.147875] [<805e626c>] (dev_watchdog) from [<801826c8>] (call_timer_fn+0x44/0x208)
[   22.155613] [<801826c8>] (call_timer_fn) from [<80182934>] (expire_timers+0xa8/0x15c)
[   22.163437] [<80182934>] (expire_timers) from [<80182a74>] (run_timer_softirq+0x8c/0x164)
[   22.171610] [<80182a74>] (run_timer_softirq) from [<80124a7c>] (__do_softirq+0xac/0x3f0)
[   22.179696] [<80124a7c>] (__do_softirq) from [<80125124>] (irq_exit+0xf0/0x158)
[   22.187003] [<80125124>] (irq_exit) from [<8016ffd4>] (__handle_domain_irq+0x60/0xb8)
[   22.194828] [<8016ffd4>] (__handle_domain_irq) from [<801014c4>] (gic_handle_irq+0x4c/0x9c)
[   22.203175] [<801014c4>] (gic_handle_irq) from [<806cc48c>] (__irq_svc+0x6c/0xa8)
[   22.210648] Exception stack(0x80b01f60 to 0x80b01fa8)
[   22.215694] 1f60: 00000000 bf5c03f0 80b01fb8 8011a060 00000000 00000001 80b03c9c 80b03c2c
[   22.223865] 1f80: 80b1c045 80b1c045 00000001 00000000 80a673f0 80b01fb0 801090c0 801090c4
[   22.232032] 1fa0: 60000013 ffffffff
[   22.235520] [<806cc48c>] (__irq_svc) from [<801090c4>] (arch_cpu_idle+0x38/0x44)
[   22.242914] [<801090c4>] (arch_cpu_idle) from [<80160f00>] (cpu_startup_entry+0xd8/0x148)
[   22.251089] [<80160f00>] (cpu_startup_entry) from [<80a00c44>] (start_kernel+0x360/0x3c8)
[   22.259269] ---[ end trace e04d3944bdde616a ]---



I patched both stmmac_tso_xmit and stmmac_xmit, as instructed.
Here is the diff:

--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2090,8 +2090,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
        /* Manage tx mitigation */
        priv->tx_count_frames += nfrags + 1;
        if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
-               mod_timer(&priv->txtimer,
-                         STMMAC_COAL_TIMER(priv->tx_coal_timer));
+               if (priv->tx_count_frames == nfrags + 1)
+                       mod_timer(&priv->txtimer,
+                                 STMMAC_COAL_TIMER(priv->tx_coal_timer));
        } else {
                priv->tx_count_frames = 0;
                priv->hw->desc->set_tx_ic(desc);
@@ -2292,8 +2293,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
         */
        priv->tx_count_frames += nfrags + 1;
        if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
-               mod_timer(&priv->txtimer,
-                         STMMAC_COAL_TIMER(priv->tx_coal_timer));
+               if (priv->tx_count_frames == nfrags + 1)
+                       mod_timer(&priv->txtimer,
+                                 STMMAC_COAL_TIMER(priv->tx_coal_timer));
        } else {
                priv->tx_count_frames = 0;
                priv->hw->desc->set_tx_ic(desc);



Without your patch, I get no tx queue timeout, and ping works fine.


>
> You'll need to apply it twice as code is copy&pasted.
>
> Best regards,
> 									Pavel
>
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>
>  	 */
>  	priv->tx_count_frames += nfrags + 1;
>  	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +		if (priv->tx_count_frames == nfrags + 1)
> +			mod_timer(&priv->txtimer,
> +				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>  	} else {
>  		priv->tx_count_frames = 0;
>  		priv->hw->desc->set_tx_ic(desc);
>
>

Re: Synopsys Ethernet QoS

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3796 bytes --]

Hi!

> >> So if there is a long time before handling interrupts,
> >> I guess that it makes sense that one stream could
> >> get an advantage in the net scheduler.
> >>
> >> If I find the time, and if no one beats me to it, I will try to replace
> >> the normal timers with HR timers + a smaller default timeout.
> >>
> > 
> > Can you try something like this? Highres timers will be needed, too,
> > but this fixes the logic problem.
> > 
> > You'll need to apply it twice as code is copy&pasted.
> > 
> > Best regards,
> > 									Pavel
> > 
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > 
> >  	 */
> >  	priv->tx_count_frames += nfrags + 1;
> >  	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> > -		mod_timer(&priv->txtimer,
> > -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> > +		if (priv->tx_count_frames == nfrags + 1)
> > +			mod_timer(&priv->txtimer,
> > +				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> >  	} else {
> >  		priv->tx_count_frames = 0;
> >  		priv->hw->desc->set_tx_ic(desc);
> > 
> > 
> 
> I know that this is completely of topic, but I am facing a dificulty with
> stmmac. I have interrupts, mac well configured rx packets being received
> successfully, but TX is not working, resulting in Tx errors = Total TX packets.
> I have made a lot of debug and my conclusions is that by some reason when using
> stmmac after starting tx dma, the hw state machine enters a deadend state
> resulting in those errors. Anyone faced this trouble?

SMP or UP system? AFAICT the driver gets the memory barriers wrong. It
does not fail completely for me, but still fails rather quickly.

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e40578..641b03d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2121,11 +2205,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (mss_desc)
 		priv->hw->desc->set_tx_owner(mss_desc);
 
-	/* The own bit must be the latest setting done when prepare the
+	/* The own bit must be the latest setting done when preparing the
 	 * descriptor and then barrier is needed to make sure that
 	 * all is coherent before granting the DMA engine.
 	 */
-	smp_wmb();
+	wmb();
 
 	if (netif_msg_pktdata(priv)) {
 		pr_info("%s: curr=%d dirty=%d f=%d, e=%d, f_p=%p, nfrags %d\n",
@@ -2336,9 +2401,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 		/* The own bit must be the latest setting done when prepare the
 		 * descriptor and then barrier is needed to make sure that
-		 * all is coherent before granting the DMA engine.
+		 * all is coherent before granting access to the DMA engine.
 		 */
-		smp_wmb();
+		wmb();
 	}
 
 	netdev_sent_queue(dev, skb->len);

Plus I'd suggest... at least (hand-edited). Driver should really be
modified to use readl() when accessing memory that changes.

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e40578..641b03d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1309,6 +1323,8 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 		status = priv->hw->desc->tx_status(&priv->dev->stats,
 						      &priv->xstats, p,
 						      priv->ioaddr);
+		rmb();
+		
 		/* Check if the descriptor is owned by the DMA */
 		if (unlikely(status & tx_dma_own))
 			break;

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Synopsys Ethernet QoS

[Joao Pinto]

Hi Pavel,

Às 5:38 PM de 12/17/2016, Pavel Machek escreveu:
> Hi!
> 
>>>> So if there is a long time before handling interrupts,
>>>> I guess that it makes sense that one stream could
>>>> get an advantage in the net scheduler.
>>>>
>>>> If I find the time, and if no one beats me to it, I will try to replace
>>>> the normal timers with HR timers + a smaller default timeout.
>>>>
>>>
>>> Can you try something like this? Highres timers will be needed, too,
>>> but this fixes the logic problem.
>>>
>>> You'll need to apply it twice as code is copy&pasted.
>>>
>>> Best regards,
>>> 									Pavel
>>>
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>
>>>  	 */
>>>  	priv->tx_count_frames += nfrags + 1;
>>>  	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>>> -		mod_timer(&priv->txtimer,
>>> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>>> +		if (priv->tx_count_frames == nfrags + 1)
>>> +			mod_timer(&priv->txtimer,
>>> +				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>>>  	} else {
>>>  		priv->tx_count_frames = 0;
>>>  		priv->hw->desc->set_tx_ic(desc);
>>>
>>>
>>
>> I know that this is completely of topic, but I am facing a dificulty with
>> stmmac. I have interrupts, mac well configured rx packets being received
>> successfully, but TX is not working, resulting in Tx errors = Total TX packets.
>> I have made a lot of debug and my conclusions is that by some reason when using
>> stmmac after starting tx dma, the hw state machine enters a deadend state
>> resulting in those errors. Anyone faced this trouble?
> 
> SMP or UP system? AFAICT the driver gets the memory barriers wrong. It
> does not fail completely for me, but still fails rather quickly.

I was able to put my setup receiving and transmiting packets. The problem is
that because my hw controller supports multi-queues, I had to enable the TX
queue 0 in order for it to work.

Now I am facing a new problem. My DUT with stmmac does not answer to ARP
messages. When I execute a "ping" the other machine in the nework executes a ARP
message asking "who is the owner of IP XXXX" which my DUT does not reply. I used
wireshark and the ARP messages are reaching the DUT machine, but for some reason
it does not reply. Did you ever face this problem when using stmmac?

When using my internal driver, it works perfectly, so some detail in the stmmac
I suppose.

Tahnks.

> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3e40578..641b03d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2121,11 +2205,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	if (mss_desc)
>  		priv->hw->desc->set_tx_owner(mss_desc);
>  
> -	/* The own bit must be the latest setting done when prepare the
> +	/* The own bit must be the latest setting done when preparing the
>  	 * descriptor and then barrier is needed to make sure that
>  	 * all is coherent before granting the DMA engine.
>  	 */
> -	smp_wmb();
> +	wmb();
>  
>  	if (netif_msg_pktdata(priv)) {
>  		pr_info("%s: curr=%d dirty=%d f=%d, e=%d, f_p=%p, nfrags %d\n",
> @@ -2336,9 +2401,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  		/* The own bit must be the latest setting done when prepare the
>  		 * descriptor and then barrier is needed to make sure that
> -		 * all is coherent before granting the DMA engine.
> +		 * all is coherent before granting access to the DMA engine.
>  		 */
> -		smp_wmb();
> +		wmb();
>  	}
>  
>  	netdev_sent_queue(dev, skb->len);
> 
> Plus I'd suggest... at least (hand-edited). Driver should really be
> modified to use readl() when accessing memory that changes.
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3e40578..641b03d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1309,6 +1323,8 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
>  		status = priv->hw->desc->tx_status(&priv->dev->stats,
>  						      &priv->xstats, p,
>  						      priv->ioaddr);
> +		rmb();
> +		
>  		/* Check if the descriptor is owned by the DMA */
>  		if (unlikely(status & tx_dma_own))
>  			break;
> 
> 									Pavel
> 

Re: Synopsys Ethernet QoS

[Joao Pinto]

Às 10:55 AM de 12/19/2016, Joao Pinto escreveu:
> 
> Hi Pavel,
> 
> Às 5:38 PM de 12/17/2016, Pavel Machek escreveu:
>> Hi!
>>
>>>>> So if there is a long time before handling interrupts,
>>>>> I guess that it makes sense that one stream could
>>>>> get an advantage in the net scheduler.
>>>>>
>>>>> If I find the time, and if no one beats me to it, I will try to replace
>>>>> the normal timers with HR timers + a smaller default timeout.
>>>>>
>>>>
>>>> Can you try something like this? Highres timers will be needed, too,
>>>> but this fixes the logic problem.
>>>>
>>>> You'll need to apply it twice as code is copy&pasted.
>>>>
>>>> Best regards,
>>>> 									Pavel
>>>>
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>>>
>>>>  	 */
>>>>  	priv->tx_count_frames += nfrags + 1;
>>>>  	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>>>> -		mod_timer(&priv->txtimer,
>>>> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>>>> +		if (priv->tx_count_frames == nfrags + 1)
>>>> +			mod_timer(&priv->txtimer,
>>>> +				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>>>>  	} else {
>>>>  		priv->tx_count_frames = 0;
>>>>  		priv->hw->desc->set_tx_ic(desc);
>>>>
>>>>
>>>
>>> I know that this is completely of topic, but I am facing a dificulty with
>>> stmmac. I have interrupts, mac well configured rx packets being received
>>> successfully, but TX is not working, resulting in Tx errors = Total TX packets.
>>> I have made a lot of debug and my conclusions is that by some reason when using
>>> stmmac after starting tx dma, the hw state machine enters a deadend state
>>> resulting in those errors. Anyone faced this trouble?
>>
>> SMP or UP system? AFAICT the driver gets the memory barriers wrong. It
>> does not fail completely for me, but still fails rather quickly.
> 
> I was able to put my setup receiving and transmiting packets. The problem is
> that because my hw controller supports multi-queues, I had to enable the TX
> queue 0 in order for it to work.
> 
> Now I am facing a new problem. My DUT with stmmac does not answer to ARP
> messages. When I execute a "ping" the other machine in the nework executes a ARP
> message asking "who is the owner of IP XXXX" which my DUT does not reply. I used
> wireshark and the ARP messages are reaching the DUT machine, but for some reason
> it does not reply. Did you ever face this problem when using stmmac?
> 
> When using my internal driver, it works perfectly, so some detail in the stmmac
> I suppose.
> 
> Tahnks.

Forgot to say that I am using a PC with SMP and a PCI link to the controller.

Thanks.

> 
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 3e40578..641b03d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -2121,11 +2205,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>>  	if (mss_desc)
>>  		priv->hw->desc->set_tx_owner(mss_desc);
>>  
>> -	/* The own bit must be the latest setting done when prepare the
>> +	/* The own bit must be the latest setting done when preparing the
>>  	 * descriptor and then barrier is needed to make sure that
>>  	 * all is coherent before granting the DMA engine.
>>  	 */
>> -	smp_wmb();
>> +	wmb();
>>  
>>  	if (netif_msg_pktdata(priv)) {
>>  		pr_info("%s: curr=%d dirty=%d f=%d, e=%d, f_p=%p, nfrags %d\n",
>> @@ -2336,9 +2401,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>>  
>>  		/* The own bit must be the latest setting done when prepare the
>>  		 * descriptor and then barrier is needed to make sure that
>> -		 * all is coherent before granting the DMA engine.
>> +		 * all is coherent before granting access to the DMA engine.
>>  		 */
>> -		smp_wmb();
>> +		wmb();
>>  	}
>>  
>>  	netdev_sent_queue(dev, skb->len);
>>
>> Plus I'd suggest... at least (hand-edited). Driver should really be
>> modified to use readl() when accessing memory that changes.
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 3e40578..641b03d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -1309,6 +1323,8 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
>>  		status = priv->hw->desc->tx_status(&priv->dev->stats,
>>  						      &priv->xstats, p,
>>  						      priv->ioaddr);
>> +		rmb();
>> +		
>>  		/* Check if the descriptor is owned by the DMA */
>>  		if (unlikely(status & tx_dma_own))
>>  			break;
>>
>> 									Pavel
>>
> 

Re: Synopsys Ethernet QoS

[Niklas Cassel]

On 12/13/2016 01:56 PM, Joao Pinto wrote:
> Às 12:50 PM de 12/13/2016, Lars Persson escreveu:
>>> 13 dec. 2016 kl. 13:31 skrev Niklas Cassel <Niklas.Cassel@axis.com>:
>>>
>>>
>>>
>>>> On 12/13/2016 12:49 PM, Joao Pinto wrote:
>>>> Hi Niklas,
>>>>
>>>> Às 4:25 PM de 12/12/2016, Niklas Cassel escreveu:
>>>>>> On 12/12/2016 11:19 AM, Joao Pinto wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Às 1:44 AM de 12/10/2016, Florian Fainelli escreveu:
>>>>>>>> Le 12/09/16 à 16:16, Andy Shevchenko a écrit :
>>>>>>>> On Sat, Dec 10, 2016 at 12:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>> (snip...)
>>>>
>>>>
>>>>>> @Rabin Vincent: Hi Rabin. Since Axis is more familiar with the synopsys/*qos*
>>>>>> driver would it be possible for you to make an initial analysis of what has to
>>>>>> be merged into Stmmac? This way the development would speed-up.
>>>>> I can answer that question.
>>>>>
>>>>> I've sent out 12 patches to the stmmac driver
>>>>> (all patches are included in the current net-next tree),
>>>>> with these patches the stmmac driver works properly on Axis hardware
>>>>> (we use Synopsys GMAC 4.10a synthesized with multiple TX queues).
>>>>> stmmac's DT binding has also been extended with properties that
>>>>> existed in DWC EQoS's DT binding, such as no-pbl-x8, txpbl, rxpbl.
>>>>>
>>>>> Since we have no problem updating the DTB together with the kernel,
>>>>> we will simply move to using the start using the stmmac driver,
>>>>> with stmmac's DT binding.
>>>>>
>>>>> However, I've noticed that NVIDIA has extended the DWC EQoS DT binding,
>>>>> I don't how easy it would be for them to switch to stmmac's DT binding.
>>>>> (Adding Stephen Warren to CC.)
>>>>>
>>>>> The reset sequence that Lars Persson was worried about is not an issue
>>>>> with the stmmac driver.
>>>> Great! So you saying that stmmac works great with AXIS hardware and no need to
>>>> merge specific code from AXIS' *qos* driver?
>>> Yes. From Axis point of view, we are done.
>>> stmmac works and we will move to that driver + DT binding.
>>>
>>> However, it seems like Stephen Warren will NAK if you try to remove
>>> synopsys/dwc_eth_qos.c before
>>> Documentation/devicetree/bindings/net/stmmac.txt
>>> is compatible with
>>> Documentation/devicetree/bindings/net/snps,dwc-qos-ethernet.txt
>>>
>>> You might want to sync with him. I have no idea, but perhaps they are
>>> only using a subset of all the available properties. Perhaps,
>>> only implementing what they are using might be enough, I don't know.
>>> I couldn't find their DTS in arch/arm/dts.
>>> I guess you might want to know David Miller's opinion,
>>> since he's the one who decides in the end.
>> I will also NACK removal of dwc_eth_qos.c until the binding is implemented elsewhere.
> Totally agree.
> @Niklas: David Miller is informed of what we are planning to do. Can you
> estimate the effort of merging the axis driver device tree bindings? If there
> was anyone from axis to do the merge would be better since you are familiar with
> it. What do you think?

Since stmmac supports glue layers, the best thing is probably
to create a new glue layer
(see for example drivers/net/ethernet/stmicro/stmmac/dwmac-meson.c
with matching Documentation/devicetree/bindings/net/meson-dwmac.txt).

That way we don't have to "contaminate" the generic code in
stmmac_platform.c and dwmac-generic.c.

The only code needed in the glue driver would be the code to parse the
devicetree properties specific to dwc_eth_qos.c.
Hence, the amount of code you will have to write will be very limited.
Writing the code will probably be quick, but since you will have to
fix review comments etc., I would estimate it to be around 1-2 days work.

Since we have already moved to stmmac's DT binding,
we don't really care about the old binding, but I will gladly
help you by performing code reviews if you would like.


>
>>
>>>>>
>>>>>
>>>>> There are some performance problems with the stmmac driver though:
>>>>>
>>>>> When running iperf3 with 3 streams:
>>>>> iperf3 -c 192.168.0.90 -P 3 -t 30
>>>>> iperf3 -c 192.168.0.90 -P 3 -t 30 -R
>>>>>
>>>>> I get really bad fairness between the streams.
>>>>>
>>>>> This appears to be an issue with how TX IRQ coalescing is implemented in stmmac.
>>>>> Disabling TX IRQ coalescing in the stmmac driver makes the problem go away.
>>>>> We have a local patch that implements TX IRQ coalescing in the dwceqos driver,
>>>>> and we don't see the same problem.
>>>>>
>>>>> Also netperf TCP_RR and UDP_RR gives really bad results compared to the
>>>>> dwceqos driver (without IRQ coalescing).
>>>>> 2000 transactions/sec vs 9000 transactions/sec.
>>>>> Turning TX IRQ coalescing off and RX interrupt watchdog off in stmmac
>>>>> gives the same performance. I guess it's a trade off, low CPU usage
>>>>> vs low latency, so I don't know how important TCP_RR/UDP_RR really is.
>>>>>
>>>>> The best thing would be to get a good working TX IRQ coalesce
>>>>> implementation with HR timers in stmmac.
>>>>> Perhaps it should also be investigated if the RX interrupt watchdog
>>>>> timeout should have a lower default value.
>>>>>
>>>>>
>>>>>
>>>>>> Thanks to all.
>>>>>>
>>>>>> Joao

Re: Synopsys Ethernet QoS

[Niklas Cassel]

On 12/14/2016 01:57 PM, Pavel Machek wrote:
> Hi!
>
>> So if there is a long time before handling interrupts,
>> I guess that it makes sense that one stream could
>> get an advantage in the net scheduler.
>>
>> If I find the time, and if no one beats me to it, I will try to replace
>> the normal timers with HR timers + a smaller default timeout.
>>
> Can you try something like this? Highres timers will be needed, too,
> but this fixes the logic problem.
>
> You'll need to apply it twice as code is copy&pasted.
>
> Best regards,

Hello Pavel

If I first apply your other fix "stmmac: fix memory barriers",
then I can apply this fix without getting a netdev watchdog timeout on tx queue0,
and everything appears to work as it should.

Regarding to fairness, I can't really see a difference, with or without your patch.
I've noticed that for TX, the streams actually stabilize after 5 seconds or so,
but with the default test length of 10 seconds, it's easy to get confused
by the test result summary. So I guess from a fairness point of view,
TX is not really a problem.

For RX fairness, it is very much a real issue. The streams never stabilize.
One key difference is that RX coalescing is implemented by using the
RX watchdog.
Here is an iperf3 run that went for 30 seconds:

[  4]   0.00-30.00  sec  1.54 GBytes   440 Mbits/sec    0             sender
[  4]   0.00-30.00  sec  1.54 GBytes   440 Mbits/sec                  receiver
[  6]   0.00-30.00  sec   600 MBytes   168 Mbits/sec    0             sender
[  6]   0.00-30.00  sec   599 MBytes   168 Mbits/sec                  receiver
[  8]   0.00-30.00  sec  1.17 GBytes   334 Mbits/sec    0             sender
[  8]   0.00-30.00  sec  1.17 GBytes   334 Mbits/sec                  receiver
[SUM]   0.00-30.00  sec  3.29 GBytes   942 Mbits/sec    0             sender
[SUM]   0.00-30.00  sec  3.29 GBytes   942 Mbits/sec                  receiver


> 									Pavel
>
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>
>  	 */
>  	priv->tx_count_frames += nfrags + 1;
>  	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> -		mod_timer(&priv->txtimer,
> -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> +		if (priv->tx_count_frames == nfrags + 1)
> +			mod_timer(&priv->txtimer,
> +				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
>  	} else {
>  		priv->tx_count_frames = 0;
>  		priv->hw->desc->set_tx_ic(desc);
>
>