lkml.org 
[lkml]   [2016]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: stmmac ethernet in kernel 4.9-rc6: coalescing related pauses.
    From
    Date
    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
    >>
    >

    \
     
     \ /
      Last update: 2016-12-02 15:26    [W:9.045 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site