Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1534736 > unrolled thread

Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 09:30 +0100
    Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-02 09:50 +0100
      Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 10:50 +0100
        Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-02 13:40 +0100
          Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 15:00 +0100
            Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Alexandre Torgue <alexandre.torgue@st.com> - 2016-12-02 15:30 +0100
              Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 17:10 +0100
                Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 13:40 +0100
            Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 13:10 +0100
        Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 11:40 +0100
          Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. "Lino Sanfilippo" <LinoSanfilippo@gmx.de> - 2016-12-05 12:50 +0100
            Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 23:20 +0100
              Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-05 23:40 +0100
                Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 23:50 +0100
                  Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-06 00:00 +0100
                    Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-06 00:20 +0100
      Aw: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related  pauses. "Lino Sanfilippo" <LinoSanfilippo@gmx.de> - 2016-12-02 15:10 +0100
        [RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related  pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-07 13:40 +0100
          Re: [RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing  related pauses. Lino Sanfilippo <lsanfil@marvell.com> - 2016-12-07 14:20 +0100
    Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Giuseppe CAVALLARO <peppe.cavallaro@st.com> - 2016-12-02 09:50 +0100
    Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses. Pavel Machek <pavel@ucw.cz> - 2016-12-05 13:00 +0100

Page 1 of 2  [1] 2  Next page →

#1534736 — Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

Hello


On 11/24/2016 10:25 PM, Pavel Machek wrote:
> Hi!
>
>>>> I'm debugging strange delays during transmit in stmmac driver. They
>>>> seem to be present in 4.4 kernel (and older kernels, too). Workload is
>>>> burst of udp packets being sent, pause, burst of udp packets, ...
> ...
>>> 4.9-rc6 still has the delays. With the
>>>
>>> #define STMMAC_COAL_TX_TIMER 1000
>>> #define STMMAC_TX_MAX_FRAMES 2
>>>
>>> settings, delays go away, and driver still works. (It fails fairly
>>> fast in 4.4). Good news. But the question still is: what is going on
>>> there?
>>
>> 256 packets looks way too large for being a trigger for aborting the
>> TX coalescing timer.
>>
>> Looking more deeply into this, the driver is using non-highres timers
>> to implement the TX coalescing.  This simply cannot work.
>>
>> 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.
>>
>> I seriously think that the TX coalescing support should be ripped out
>> or disabled entirely until it is implemented properly in this
>> driver.
>
> Ok, I'd disable coalescing, but could not figure it out till. What is
> generic way to do that?
>
> It seems only thing stmmac_tx_timer() does is calling
> stmmac_tx_clean(), which reclaims tx_skbuff[] entries. It should be
> possible to do that explicitely, without delay, but it stops working
> completely if I attempt to do that.
>
> On a side note, stmmac_poll() does stmmac_enable_dma_irq() while
> stmmac_dma_interrupt() disables interrupts. But I don't see any
> protection between the two, so IMO it could race and we'd end up
> without polling or interrupts...


the idea behind the TX mitigation is to mix the interrupt and
timer and this approach gave us real benefit in terms
of performances and CPU usage (especially on SH4-200/SH4-300 platforms
based).
In the ring, some descriptors can raise the irq (according to a
threshold) and set the IC bit. In this path, the NAPI  poll will be
scheduled.
But there is a timer that can run (and we experimented that no high
resolution is needed) to clear the tx resources.
Concerning the lock protection, we had reviewed long time ago and
IIRC, no raise condition should be present. Open to review it, again!

So, welcome any other schema and testing on platforms supported.


Hoping this summary can help.

Peppe


>
> Thanks and best regards,
> 									Pavel
>

[toc] | [next] | [standalone]

#1534741

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >>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.
> >>
> >>I seriously think that the TX coalescing support should be ripped out
> >>or disabled entirely until it is implemented properly in this
> >>driver.
> >
> >Ok, I'd disable coalescing, but could not figure it out till. What is
> >generic way to do that?
> >
> >It seems only thing stmmac_tx_timer() does is calling
> >stmmac_tx_clean(), which reclaims tx_skbuff[] entries. It should be
> >possible to do that explicitely, without delay, but it stops working
> >completely if I attempt to do that.
> >
> >On a side note, stmmac_poll() does stmmac_enable_dma_irq() while
> >stmmac_dma_interrupt() disables interrupts. But I don't see any
> >protection between the two, so IMO it could race and we'd end up
> >without polling or interrupts...
> 
> 
> the idea behind the TX mitigation is to mix the interrupt and
> timer and this approach gave us real benefit in terms
> of performances and CPU usage (especially on SH4-200/SH4-300 platforms
> based).

Well, if you have a workload that sends and receive packets, it tends
to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
like that -- it is "sending packets at 3MB/sec, receiving none". So
the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
and then we run out of transmit descriptors, and then 40msec passes,
and then we clean them. Bad.

And that's why low-res timers do not cut it.

> In the ring, some descriptors can raise the irq (according to a
> threshold) and set the IC bit. In this path, the NAPI  poll will be
> scheduled.

Not NAPI poll but stmmac_tx_timer(), right? 

> But there is a timer that can run (and we experimented that no high
> resolution is needed) to clear the tx resources.
> Concerning the lock protection, we had reviewed long time ago and
> IIRC, no raise condition should be present. Open to review it,
> again!

Well, I certainly like the fact that we are talking :-).

And yes, I have some questions.

There's nothing that protect stmmac_poll() from running concurently
with stmmac_dma_interrupt(), right?

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

[toc] | [prev] | [next] | [standalone]

#1534775

Hi Pavel

On 12/2/2016 9:45 AM, Pavel Machek wrote:
> Hi!
>
>>>> 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.
>>>>
>>>> I seriously think that the TX coalescing support should be ripped out
>>>> or disabled entirely until it is implemented properly in this
>>>> driver.
>>>
>>> Ok, I'd disable coalescing, but could not figure it out till. What is
>>> generic way to do that?
>>>
>>> It seems only thing stmmac_tx_timer() does is calling
>>> stmmac_tx_clean(), which reclaims tx_skbuff[] entries. It should be
>>> possible to do that explicitely, without delay, but it stops working
>>> completely if I attempt to do that.
>>>
>>> On a side note, stmmac_poll() does stmmac_enable_dma_irq() while
>>> stmmac_dma_interrupt() disables interrupts. But I don't see any
>>> protection between the two, so IMO it could race and we'd end up
>>> without polling or interrupts...
>>
>>
>> the idea behind the TX mitigation is to mix the interrupt and
>> timer and this approach gave us real benefit in terms
>> of performances and CPU usage (especially on SH4-200/SH4-300 platforms
>> based).
>
> Well, if you have a workload that sends and receive packets, it tends
> to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
> like that -- it is "sending packets at 3MB/sec, receiving none". So
> the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
> and then we run out of transmit descriptors, and then 40msec passes,
> and then we clean them. Bad.
>
> And that's why low-res timers do not cut it.

in that case, I expect that the tuning of the driver could help you.
I mean, by using ethtool, it could be enough to set the IC bit on all
the descriptors. You should touch the tx_coal_frames.

Then you can use ethtool -S to monitor the status.

We had experimented this tuning on STB IP where just datagrams
had to send externally. To be honest, although we had seen
better results w/o any timer, we kept this approach enabled
because the timer was fast enough to cover our tests on SH4 boxes.

FYI, stmmac doesn't implement adaptive algo.

>
>> In the ring, some descriptors can raise the irq (according to a
>> threshold) and set the IC bit. In this path, the NAPI  poll will be
>> scheduled.
>
> Not NAPI poll but stmmac_tx_timer(), right?

in the xmit according the the threshold the timer is started or the
interrupt is set inside the descriptor.
Then stmmac_tx_clean will be always called and, if you see the flow,
no irqlock protection is needed!

>
>> But there is a timer that can run (and we experimented that no high
>> resolution is needed) to clear the tx resources.
>> Concerning the lock protection, we had reviewed long time ago and
>> IIRC, no raise condition should be present. Open to review it,
>> again!
>
> Well, I certainly like the fact that we are talking :-).
>
> And yes, I have some questions.
>
> There's nothing that protect stmmac_poll() from running concurently
> with stmmac_dma_interrupt(), right?

This is not necessary.

Best Regards
peppe

>
> Best regards,
> 									Pavel
>

[toc] | [prev] | [next] | [standalone]

#1534883

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >Well, if you have a workload that sends and receive packets, it tends
> >to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
> >like that -- it is "sending packets at 3MB/sec, receiving none". So
> >the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
> >and then we run out of transmit descriptors, and then 40msec passes,
> >and then we clean them. Bad.
> >
> >And that's why low-res timers do not cut it.
> 
> in that case, I expect that the tuning of the driver could help you.
> I mean, by using ethtool, it could be enough to set the IC bit on all
> the descriptors. You should touch the tx_coal_frames.
> 
> Then you can use ethtool -S to monitor the status.

Yes, I did something similar. Unfortnunately that meant crash within
minutes, at least with 4.4 kernel. (If you know what was fixed between
4.4 and 4.9, that would be helpful).

> We had experimented this tuning on STB IP where just datagrams
> had to send externally. To be honest, although we had seen
> better results w/o any timer, we kept this approach enabled
> because the timer was fast enough to cover our tests on SH4 boxes.

Please reply to David, and explain how it is supposed to
work... because right now it does not. 40 msec delays are not
acceptable in default configuration.

> >>In the ring, some descriptors can raise the irq (according to a
> >>threshold) and set the IC bit. In this path, the NAPI  poll will be
> >>scheduled.
> >
> >Not NAPI poll but stmmac_tx_timer(), right?
> 
> in the xmit according the the threshold the timer is started or the
> interrupt is set inside the descriptor.
> Then stmmac_tx_clean will be always called and, if you see the flow,
> no irqlock protection is needed!

Agreed that no irqlock protection is needed if we rely on napi and timers.

> >>Concerning the lock protection, we had reviewed long time ago and
> >>IIRC, no raise condition should be present. Open to review it,
> >>again!
...
> >There's nothing that protect stmmac_poll() from running concurently
> >with stmmac_dma_interrupt(), right?
> 
> This is not necessary.

dma_interrupt accesses shared priv->xstats; variables are of type
unsigned long (not atomic_t), yet they are accesssed from interrupt
context and from stmmac_ethtool without any locking. That can result
in broken statistics AFAICT.

Please take another look. As far as I can tell, you can have two cpus
at #1 and #2 in the code, at the same time. It looks like napi_... has
some atomic opertions inside so that looks safe at the first look. But
I'm not sure if they also include enough memory barriers to make it
safe...?


static void stmmac_dma_interrupt(struct stmmac_priv *priv)
{
...
        status = priv->hw->dma->dma_interrupt(priv->ioaddr, &priv->xstats);
        if (likely((status & handle_rx)) || (status & handle_tx)) {
                if (likely(napi_schedule_prep(&priv->napi))) {
#1
                        stmmac_disable_dma_irq(priv);
                        __napi_schedule(&priv->napi);
                }
        }


static int stmmac_poll(struct napi_struct *napi, int budget)
{
        struct stmmac_priv *priv = container_of(napi, struct stmmac_priv, napi);
        int work_done = 0;

        priv->xstats.napi_poll++;
        stmmac_tx_clean(priv);

        work_done = stmmac_rx(priv, budget);
	if (work_done < budget) {
                napi_complete(napi);
#2
	        stmmac_enable_dma_irq(priv);
        }
        return work_done;
}


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

[toc] | [prev] | [next] | [standalone]

#1534930

On 12/2/2016 1:32 PM, Pavel Machek wrote:
> Hi!
>
>>> Well, if you have a workload that sends and receive packets, it tends
>>> to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
>>> like that -- it is "sending packets at 3MB/sec, receiving none". So
>>> the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
>>> and then we run out of transmit descriptors, and then 40msec passes,
>>> and then we clean them. Bad.
>>>
>>> And that's why low-res timers do not cut it.
>>
>> in that case, I expect that the tuning of the driver could help you.
>> I mean, by using ethtool, it could be enough to set the IC bit on all
>> the descriptors. You should touch the tx_coal_frames.
>>
>> Then you can use ethtool -S to monitor the status.
>
> Yes, I did something similar. Unfortnunately that meant crash within
> minutes, at least with 4.4 kernel. (If you know what was fixed between
> 4.4 and 4.9, that would be helpful).

4.4 has no GMAC4 support.
Alex, do you remember any patches to fix that?

>> We had experimented this tuning on STB IP where just datagrams
>> had to send externally. To be honest, although we had seen
>> better results w/o any timer, we kept this approach enabled
>> because the timer was fast enough to cover our tests on SH4 boxes.
>
> Please reply to David, and explain how it is supposed to
> work... because right now it does not. 40 msec delays are not
> acceptable in default configuration.

I mean, that on UP and SMP system this schema helped
to improve the performance saving CPU on my side and this has been
tested since a long time (~4 years).
I tested something similar to yours where unidirectional traffic
with limited throughput was needed and I can confirm you that
tuning/removing coalesce parameters this helped. The tuning I decided
to keep in the driver was suitable in several user cases and if now
you have problems or you want to review it I can just confirm that
there are no problems on my side. If you want to simply the logic
around the tx process and remove timer on official driver I can accept
that. I will just ask you uto double check if the throughput and
CPU usage when request max throughput (better if on GiGa setup) has
no regressions.
Otherwise we could start thinking about adaptive schema if feasible.

>>>> In the ring, some descriptors can raise the irq (according to a
>>>> threshold) and set the IC bit. In this path, the NAPI  poll will be
>>>> scheduled.
>>>
>>> Not NAPI poll but stmmac_tx_timer(), right?
>>
>> in the xmit according the the threshold the timer is started or the
>> interrupt is set inside the descriptor.
>> Then stmmac_tx_clean will be always called and, if you see the flow,
>> no irqlock protection is needed!
>
> Agreed that no irqlock protection is needed if we rely on napi and timers.

ok

>
>>>> Concerning the lock protection, we had reviewed long time ago and
>>>> IIRC, no raise condition should be present. Open to review it,
>>>> again!
> ...
>>> There's nothing that protect stmmac_poll() from running concurently
>>> with stmmac_dma_interrupt(), right?
>>
>> This is not necessary.
>
> dma_interrupt accesses shared priv->xstats; variables are of type
> unsigned long (not atomic_t), yet they are accesssed from interrupt
> context and from stmmac_ethtool without any locking. That can result
> in broken statistics AFAICT.

ok we can check this and welcome patches and I'd prefer to
remove xstats from critical part of the code like ISR (that
comes from old story of the driver).

>
> Please take another look. As far as I can tell, you can have two cpus
> at #1 and #2 in the code, at the same time. It looks like napi_... has
> some atomic opertions inside so that looks safe at the first look. But
> I'm not sure if they also include enough memory barriers to make it
> safe...?

Although I have never reproduced related issues on SMP platforms
due to reordering of memory operations but, as said above, welcome
review on this especially if you are seeing problems when manage NAPI.

FYI, the only memory barrier you will see in the driver are about the
OWN_BIT setting till now.

> static void stmmac_dma_interrupt(struct stmmac_priv *priv)
> {
> ...
>         status = priv->hw->dma->dma_interrupt(priv->ioaddr, &priv->xstats);
>         if (likely((status & handle_rx)) || (status & handle_tx)) {
>                 if (likely(napi_schedule_prep(&priv->napi))) {
> #1
>                         stmmac_disable_dma_irq(priv);
>                         __napi_schedule(&priv->napi);
>                 }
>         }
>
>
> static int stmmac_poll(struct napi_struct *napi, int budget)
> {
>         struct stmmac_priv *priv = container_of(napi, struct stmmac_priv, napi);
>         int work_done = 0;
>
>         priv->xstats.napi_poll++;
>         stmmac_tx_clean(priv);
>
>         work_done = stmmac_rx(priv, budget);
> 	if (work_done < budget) {
>                 napi_complete(napi);
> #2
> 	        stmmac_enable_dma_irq(priv);
>         }

hmm, I have to check (and refresh my memory) but the driver
uses the napi_schedule_prep.

Regards

Peppe

>         return work_done;
> }
>
>
> Best regards,
> 									Pavel
>

[toc] | [prev] | [next] | [standalone]

#1534977

Hi Pavel and Peppe,

On 12/02/2016 02:51 PM, Giuseppe CAVALLARO wrote:
> On 12/2/2016 1:32 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> Well, if you have a workload that sends and receive packets, it tends
>>>> to work ok, as you do tx_clean() in stmmac_poll(). My workload is not
>>>> like that -- it is "sending packets at 3MB/sec, receiving none". So
>>>> the stmmac_tx_timer() is rescheduled and rescheduled and rescheduled,
>>>> and then we run out of transmit descriptors, and then 40msec passes,
>>>> and then we clean them. Bad.
>>>>
>>>> And that's why low-res timers do not cut it.
>>>
>>> in that case, I expect that the tuning of the driver could help you.
>>> I mean, by using ethtool, it could be enough to set the IC bit on all
>>> the descriptors. You should touch the tx_coal_frames.
>>>
>>> Then you can use ethtool -S to monitor the status.
>>
>> Yes, I did something similar. Unfortnunately that meant crash within
>> minutes, at least with 4.4 kernel. (If you know what was fixed between
>> 4.4 and 4.9, that would be helpful).
>
> 4.4 has no GMAC4 support.
> Alex, do you remember any patches to fix that?

No sorry Peppe.

Pavel,

Sorry but I'm a little bit confused. I'm dropped in some mails without 
historic. I see cleanup, coalescence issue and TSO question.
What is your main issue? Are you working on gmac4 or 3.x ?
Can you refresh a little bit the story please ?

Regards
Alex
>
>>> We had experimented this tuning on STB IP where just datagrams
>>> had to send externally. To be honest, although we had seen
>>> better results w/o any timer, we kept this approach enabled
>>> because the timer was fast enough to cover our tests on SH4 boxes.
>>
>> Please reply to David, and explain how it is supposed to
>> work... because right now it does not. 40 msec delays are not
>> acceptable in default configuration.
>
> I mean, that on UP and SMP system this schema helped
> to improve the performance saving CPU on my side and this has been
> tested since a long time (~4 years).
> I tested something similar to yours where unidirectional traffic
> with limited throughput was needed and I can confirm you that
> tuning/removing coalesce parameters this helped. The tuning I decided
> to keep in the driver was suitable in several user cases and if now
> you have problems or you want to review it I can just confirm that
> there are no problems on my side. If you want to simply the logic
> around the tx process and remove timer on official driver I can accept
> that. I will just ask you uto double check if the throughput and
> CPU usage when request max throughput (better if on GiGa setup) has
> no regressions.
> Otherwise we could start thinking about adaptive schema if feasible.
>
>>>>> In the ring, some descriptors can raise the irq (according to a
>>>>> threshold) and set the IC bit. In this path, the NAPI  poll will be
>>>>> scheduled.
>>>>
>>>> Not NAPI poll but stmmac_tx_timer(), right?
>>>
>>> in the xmit according the the threshold the timer is started or the
>>> interrupt is set inside the descriptor.
>>> Then stmmac_tx_clean will be always called and, if you see the flow,
>>> no irqlock protection is needed!
>>
>> Agreed that no irqlock protection is needed if we rely on napi and
>> timers.
>
> ok
>
>>
>>>>> Concerning the lock protection, we had reviewed long time ago and
>>>>> IIRC, no raise condition should be present. Open to review it,
>>>>> again!
>> ...
>>>> There's nothing that protect stmmac_poll() from running concurently
>>>> with stmmac_dma_interrupt(), right?
>>>
>>> This is not necessary.
>>
>> dma_interrupt accesses shared priv->xstats; variables are of type
>> unsigned long (not atomic_t), yet they are accesssed from interrupt
>> context and from stmmac_ethtool without any locking. That can result
>> in broken statistics AFAICT.
>
> ok we can check this and welcome patches and I'd prefer to
> remove xstats from critical part of the code like ISR (that
> comes from old story of the driver).
>
>>
>> Please take another look. As far as I can tell, you can have two cpus
>> at #1 and #2 in the code, at the same time. It looks like napi_... has
>> some atomic opertions inside so that looks safe at the first look. But
>> I'm not sure if they also include enough memory barriers to make it
>> safe...?
>
> Although I have never reproduced related issues on SMP platforms
> due to reordering of memory operations but, as said above, welcome
> review on this especially if you are seeing problems when manage NAPI.
>
> FYI, the only memory barrier you will see in the driver are about the
> OWN_BIT setting till now.
>
>> static void stmmac_dma_interrupt(struct stmmac_priv *priv)
>> {
>> ...
>>         status = priv->hw->dma->dma_interrupt(priv->ioaddr,
>> &priv->xstats);
>>         if (likely((status & handle_rx)) || (status & handle_tx)) {
>>                 if (likely(napi_schedule_prep(&priv->napi))) {
>> #1
>>                         stmmac_disable_dma_irq(priv);
>>                         __napi_schedule(&priv->napi);
>>                 }
>>         }
>>
>>
>> static int stmmac_poll(struct napi_struct *napi, int budget)
>> {
>>         struct stmmac_priv *priv = container_of(napi, struct
>> stmmac_priv, napi);
>>         int work_done = 0;
>>
>>         priv->xstats.napi_poll++;
>>         stmmac_tx_clean(priv);
>>
>>         work_done = stmmac_rx(priv, budget);
>>     if (work_done < budget) {
>>                 napi_complete(napi);
>> #2
>>             stmmac_enable_dma_irq(priv);
>>         }
>
> hmm, I have to check (and refresh my memory) but the driver
> uses the napi_schedule_prep.
>
> Regards
>
> Peppe
>
>>         return work_done;
>> }
>>
>>
>> Best regards,
>>                                     Pavel
>>
>

[toc] | [prev] | [next] | [standalone]

#1535047

Hi Alex

On 12/2/2016 3:26 PM, Alexandre Torgue wrote:
>> 4.4 has no GMAC4 support.
>> Alex, do you remember any patches to fix that?
>
> No sorry Peppe.
>
> Pavel,
>
> Sorry but I'm a little bit confused. I'm dropped in some mails without
> historic. I see cleanup, coalescence issue and TSO question.
> What is your main issue? Are you working on gmac4 or 3.x ?
> Can you refresh a little bit the story please ?

let me try to do a sum, please Pavel feel free to correct me.

There are some open points about the tx mitigation schema
that we are trying to detail and eventually tune or change
(but keeping the same performance on other user-case).

In particular, the test case that is raising problem is
an unicast tx bench.
I suggested Pavel to tune coalesce (IC bit settings) via
ethtool and monitor stats but he is getting problems (maybe
due to lock).

IIUC problems are mainly on new kernel and not on 4.4 where
the gmac4 is missing. Please Pavel, could you confirm?

Then, there are some open points about lock protections
for xstat and Pavel is getting some problem on SMP.
I do think that we need to review that. This also could
improve the code in critical parts.

Also there are some other discussion about the lock
protection on NAPI still under discussion. I have not
clear if in this case Pavel is getting strange behavior.

Regards
Peppe

[toc] | [prev] | [next] | [standalone]

#1536062

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >Sorry but I'm a little bit confused. I'm dropped in some mails without
> >historic. I see cleanup, coalescence issue and TSO question.
> >What is your main issue? Are you working on gmac4 or 3.x ?
> >Can you refresh a little bit the story please ?
> 
> let me try to do a sum, please Pavel feel free to correct me.
> 
> There are some open points about the tx mitigation schema
> that we are trying to detail and eventually tune or change
> (but keeping the same performance on other user-case).
> 
> In particular, the test case that is raising problem is
> an unicast tx bench.
> I suggested Pavel to tune coalesce (IC bit settings) via
> ethtool and monitor stats but he is getting problems (maybe
> due to lock).
> 
> IIUC problems are mainly on new kernel and not on 4.4 where
> the gmac4 is missing. Please Pavel, could you confirm?

Actually no, it is the other way around. I can get 4.9 to work with
some tuning. 4.4 likes to crash when tx coalesce is enabled with
shorter than 40 msec timeout. (It crashes with default settings, too,
but that takes too long to reproduce.)

> Also there are some other discussion about the lock
> protection on NAPI still under discussion. I have not
> clear if in this case Pavel is getting strange behavior.

Yep, locking is broken in more than one place. I believe I understand
what some problems are. Let me prepare the patches.

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

[toc] | [prev] | [next] | [standalone]

#1536032

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >>We had experimented this tuning on STB IP where just datagrams
> >>had to send externally. To be honest, although we had seen
> >>better results w/o any timer, we kept this approach enabled
> >>because the timer was fast enough to cover our tests on SH4 boxes.
> >
> >Please reply to David, and explain how it is supposed to
> >work... because right now it does not. 40 msec delays are not
> >acceptable in default configuration.
> 
> I mean, that on UP and SMP system this schema helped
> to improve the performance saving CPU on my side and this has been
> tested since a long time (~4 years).
> I tested something similar to yours where unidirectional traffic
> with limited throughput was needed and I can confirm you that
> tuning/removing coalesce parameters this helped. The tuning I decided
> to keep in the driver was suitable in several user cases and if now
> you have problems or you want to review it I can just confirm that
> there are no problems on my side. If you want to simply the logic
> around the tx process and remove timer on official driver I can accept
> that. I will just ask you uto double check if the throughput and
> CPU usage when request max throughput (better if on GiGa setup) has
> no regressions.
> Otherwise we could start thinking about adaptive schema if feasible.

Ok, so you see the issue. Good.

See the other email to description how it could be fixed... the logic
is broken. How it was not discovered for 4 years is mystery to me.
 
> >Agreed that no irqlock protection is needed if we rely on napi and timers.
> 
> ok

Actually I was wrong there. Another reason to disable tx coalescing
until it is fixed.

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

[toc] | [prev] | [next] | [standalone]

#1535986

[Multipart message — attachments visible in raw view] — view raw

Hi!

> >>In the ring, some descriptors can raise the irq (according to a
> >>threshold) and set the IC bit. In this path, the NAPI  poll will be
> >>scheduled.
> >
> >Not NAPI poll but stmmac_tx_timer(), right?
> 
> in the xmit according the the threshold the timer is started or the
> interrupt is set inside the descriptor.
> Then stmmac_tx_clean will be always called and, if you see the flow,
> no irqlock protection is needed!

Actually, I was wrong. irqlock protection is needed, since
stmmac_tx_clean() is called from timer, and that's interrupt context,
as you can confirm using BUG_ON(in_interrupt());

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

[toc] | [prev] | [next] | [standalone]

#1536021

Hi,

> 
> Actually, I was wrong. irqlock protection is needed, since
> stmmac_tx_clean() is called from timer, and that's interrupt context,
> as you can confirm using BUG_ON(in_interrupt());
> 

in_interrupt() can mean both softirq and hardirq context. In this case it
means softirq. So I guess you were right before, and no irq locking is needed.

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1536481

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > 
> > Actually, I was wrong. irqlock protection is needed, since
> > stmmac_tx_clean() is called from timer, and that's interrupt context,
> > as you can confirm using BUG_ON(in_interrupt());
> > 
> 
> in_interrupt() can mean both softirq and hardirq context. In this case it
> means softirq. So I guess you were right before, and no irq locking is needed.

Are you absolutely sure? Because my testing seems to indicate
otherwise (but I may have made a mistake).

According to

https://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/c214.html

we need spin_lock_bh at minimum, as we are locking user context
against timer.

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

[toc] | [prev] | [next] | [standalone]

#1536488

Hi Pavel,

On 05.12.2016 23:02, Pavel Machek wrote:
> 
> we need spin_lock_bh at minimum, as we are locking user context
> against timer.
> 
> Best regards,
> 									Pavel
> 

I was referring to stmmac_tx_clean() which AFAICS is only called from softirq context,
(one time in the timer handler and one time in napi poll handler) so a spin_lock() should
be sufficient. I cant see how this is called from userspace. If it were, a spin_lock_bh() had
to be used, of course.

Regards,
Lino 

[toc] | [prev] | [next] | [standalone]

#1536501

[Multipart message — attachments visible in raw view] — view raw

On Mon 2016-12-05 23:37:09, Lino Sanfilippo wrote:
> Hi Pavel,
> 
> On 05.12.2016 23:02, Pavel Machek wrote:
> > 
> > we need spin_lock_bh at minimum, as we are locking user context
> > against timer.
> > 
> > Best regards,
> > 									Pavel
> > 
> 
> I was referring to stmmac_tx_clean() which AFAICS is only called from softirq context,
> (one time in the timer handler and one time in napi poll handler) so a spin_lock() should
> be sufficient. I cant see how this is called from userspace. If it were, a spin_lock_bh() had
> to be used, of course.

stmmac_tx_clean() shares lock with stmmac_tx() -- and that's process
context as far as I can tell. So... spin_lock_bh() at
minimum... right?

									Pavel

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

[toc] | [prev] | [next] | [standalone]

#1536506

On 05.12.2016 23:40, Pavel Machek wrote:
> On Mon 2016-12-05 23:37:09, Lino Sanfilippo wrote:
>> Hi Pavel,
>> 
>> On 05.12.2016 23:02, Pavel Machek wrote:
>> > 
>> > we need spin_lock_bh at minimum, as we are locking user context
>> > against timer.
>> > 
>> > Best regards,
>> > 									Pavel
>> > 
>> 
>> I was referring to stmmac_tx_clean() which AFAICS is only called from softirq context,
>> (one time in the timer handler and one time in napi poll handler) so a spin_lock() should
>> be sufficient. I cant see how this is called from userspace. If it were, a spin_lock_bh() had
>> to be used, of course.
> 
> stmmac_tx_clean() shares lock with stmmac_tx() -- and that's process
> context as far as I can tell. So... spin_lock_bh() at
> minimum... right?
> 
> 									Pavel
> 

You mean stmmac_xmit()? Thats also softirq AFAICT, its the TX softirq....

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1536519

> 
> You mean stmmac_xmit()? Thats also softirq AFAICT, its the TX softirq....
> 
> Regards,
> Lino
> 
> 

Hmm. netdevices.txt says:

ndo_start_xmit:
	...

	Context: Process with BHs disabled or BH (timer),
	         will be called with interrupts disabled by netconsole.

	...

If this is correct it can indeed be process context, too. However BHs are already
disabled.

[toc] | [prev] | [next] | [standalone]

#1534943 — Aw: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

Hi,


> 
> There's nothing that protect stmmac_poll() from running concurently
> with stmmac_dma_interrupt(), right?
> 

could it be that there is also another issue concerned locking?:
The tx completion handler takes the xmit_lock in case that the
netif_queue is stopped. This is AFAICS unnecessary, since both
xmit and completion handler are already synchronized by the private
tx lock. But it is IMHO also dangerous:

In the xmit handler we have the locking order
1. xmit_lock
2. private tx lock

while in the completion handler its the reverse:

1. private tx lock
2. xmit lock.

Do I miss something?


Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1537721 — [RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

[Multipart message — attachments visible in raw view] — view raw

On Fri 2016-12-02 15:05:21, Lino Sanfilippo wrote:
> Hi,
> 
> 
> > 
> > There's nothing that protect stmmac_poll() from running concurently
> > with stmmac_dma_interrupt(), right?
> > 
> 
> could it be that there is also another issue concerned locking?:
> The tx completion handler takes the xmit_lock in case that the
> netif_queue is stopped. This is AFAICS unnecessary, since both
> xmit and completion handler are already synchronized by the private
> tx lock. But it is IMHO also dangerous:
> 
> In the xmit handler we have the locking order
> 1. xmit_lock
> 2. private tx lock
> 
> while in the completion handler its the reverse:
> 
> 1. private tx lock
> 2. xmit lock.
> 
> Do I miss something?

No, it seems you are right. Something like this?

Hmm. And can priv->tx_lock be removed, as we already rely on
netif_tx_lock?

(I copied the "lock already held" annotations from forcedeth. I hope
they are right....)

Best regards,
								Pavel


commit a6f21255dfc11fcadc5062dfd0c5f3d77ca4f634
Author: Pavel <pavel@ucw.cz>
Date:   Wed Dec 7 13:29:15 2016 +0100

    Reported-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
    
    The tx completion handler takes the xmit_lock in case that the
    netif_queue is stopped. This is AFAICS unnecessary, since both
    xmit and completion handler are already synchronized by the private
    tx lock. But it is IMHO also dangerous:
    
    In the xmit handler we have the locking order
    1. xmit_lock
    2. private tx lock
    
    while in the completion handler its the reverse:
    
    1. private tx lock
    2. xmit lock.
    
    Signed-off-by: Pavel Machek <pavel@denx.de>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 982c952..5df9bb3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1380,14 +1380,9 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 
 	if (unlikely(netif_queue_stopped(priv->dev) &&
 		     stmmac_tx_avail(priv) > STMMAC_TX_THRESH)) {
-		netif_tx_lock(priv->dev);
-		if (netif_queue_stopped(priv->dev) &&
-		    stmmac_tx_avail(priv) > STMMAC_TX_THRESH) {
-			netif_dbg(priv, tx_done, priv->dev,
-				  "%s: restart transmit\n", __func__);
-			netif_wake_queue(priv->dev);
-		}
-		netif_tx_unlock(priv->dev);
+		netif_dbg(priv, tx_done, priv->dev,
+			  "%s: restart transmit\n", __func__);
+		netif_wake_queue(priv->dev);
 	}
 
 	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
@@ -1630,7 +1625,9 @@ static void stmmac_tx_timer(unsigned long data)
 {
 	struct stmmac_priv *priv = (struct stmmac_priv *)data;
 
+	netif_tx_lock_bh(priv->dev);
 	stmmac_tx_clean(priv);
+	netif_tx_unlock_bh(priv->dev);
 }
 
 /**
@@ -1994,7 +1991,8 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
  *   --------
  *
  * mss is fixed when enable tso, so w/o programming the TDES3 ctx field.
- */
+ *
+ *  Called with netif_tx_lock held.                                                                 */
 static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	u32 pay_len, mss;
@@ -2174,6 +2172,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
  *  Description : this is the tx entry point of the driver.
  *  It programs the chain or the ring and supports oversized frames
  *  and SG feature.
+ *  Called with netif_tx_lock held. 
  */
 static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -2684,7 +2683,9 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
 	int work_done = 0;
 
 	priv->xstats.napi_poll++;
+	netif_tx_lock_bh(priv->dev);
 	stmmac_tx_clean(priv);
+	netif_tx_unlock_bh(priv->dev);
 
 	work_done = stmmac_rx(priv, budget);
 	if (work_done < budget) {
@@ -2701,6 +2702,7 @@ static int stmmac_poll(struct napi_struct *napi, int budget)
  *   complete within a reasonable time. The driver will mark the error in the
  *   netdev structure and arrange for the device to be reset to a sane state
  *   in order to transmit a new packet.
+ * Called with netif_tx_lock held.
  */
 static void stmmac_tx_timeout(struct net_device *dev)
 {


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

[toc] | [prev] | [next] | [standalone]

#1537736 — Re: [RFC] Re: Re: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.

Hi,

On 07.12.2016 13:31, Pavel Machek wrote:
> On Fri 2016-12-02 15:05:21, Lino Sanfilippo wrote:
>> Hi,
>>
>>
>>>
>>> There's nothing that protect stmmac_poll() from running concurently
>>> with stmmac_dma_interrupt(), right?
>>>
>>
>> could it be that there is also another issue concerned locking?:
>> The tx completion handler takes the xmit_lock in case that the
>> netif_queue is stopped. This is AFAICS unnecessary, since both
>> xmit and completion handler are already synchronized by the private
>> tx lock. But it is IMHO also dangerous:
>>
>> In the xmit handler we have the locking order
>> 1. xmit_lock
>> 2. private tx lock
>>
>> while in the completion handler its the reverse:
>>
>> 1. private tx lock
>> 2. xmit lock.
>>
>> Do I miss something?
>
> No, it seems you are right. Something like this?
>
> Hmm. And can priv->tx_lock be removed, as we already rely on
> netif_tx_lock?
>

David wanted indeed the private lock to be removed completely. See
this thread.

https://marc.info/?l=linux-netdev&m=148072001900620&w=2

If you can wait a bit, I already have patches prepared to fix this issue in both
drivers. I want to send them as soon as I am at home (a few hours).

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1534746

+ Lino

On 12/2/2016 9:24 AM, Giuseppe CAVALLARO wrote:
>
> Hello
>
>
> On 11/24/2016 10:25 PM, Pavel Machek wrote:
>> Hi!
>>
>>>>> I'm debugging strange delays during transmit in stmmac driver. They
>>>>> seem to be present in 4.4 kernel (and older kernels, too). Workload is
>>>>> burst of udp packets being sent, pause, burst of udp packets, ...
>> ...
>>>> 4.9-rc6 still has the delays. With the
>>>>
>>>> #define STMMAC_COAL_TX_TIMER 1000
>>>> #define STMMAC_TX_MAX_FRAMES 2
>>>>
>>>> settings, delays go away, and driver still works. (It fails fairly
>>>> fast in 4.4). Good news. But the question still is: what is going on
>>>> there?
>>>
>>> 256 packets looks way too large for being a trigger for aborting the
>>> TX coalescing timer.
>>>
>>> Looking more deeply into this, the driver is using non-highres timers
>>> to implement the TX coalescing.  This simply cannot work.
>>>
>>> 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.
>>>
>>> I seriously think that the TX coalescing support should be ripped out
>>> or disabled entirely until it is implemented properly in this
>>> driver.
>>
>> Ok, I'd disable coalescing, but could not figure it out till. What is
>> generic way to do that?
>>
>> It seems only thing stmmac_tx_timer() does is calling
>> stmmac_tx_clean(), which reclaims tx_skbuff[] entries. It should be
>> possible to do that explicitely, without delay, but it stops working
>> completely if I attempt to do that.
>>
>> On a side note, stmmac_poll() does stmmac_enable_dma_irq() while
>> stmmac_dma_interrupt() disables interrupts. But I don't see any
>> protection between the two, so IMO it could race and we'd end up
>> without polling or interrupts...
>
>
> the idea behind the TX mitigation is to mix the interrupt and
> timer and this approach gave us real benefit in terms
> of performances and CPU usage (especially on SH4-200/SH4-300 platforms
> based).
> In the ring, some descriptors can raise the irq (according to a
> threshold) and set the IC bit. In this path, the NAPI  poll will be
> scheduled.
> But there is a timer that can run (and we experimented that no high
> resolution is needed) to clear the tx resources.
> Concerning the lock protection, we had reviewed long time ago and
> IIRC, no raise condition should be present. Open to review it, again!
>
> So, welcome any other schema and testing on platforms supported.
>
>
> Hoping this summary can help.
>
> Peppe
>
>
>>
>> Thanks and best regards,
>>                                     Pavel
>>
>
>

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web