Re: stmmac: turn coalescing / NAPI off in stmmac
Hi!
> >>>So patch currently looks like this (hand edited, can't be
> >>>applied, got it working few hours ago). Does it look acceptable?
> >>>
> >>>I'd prefer this to go after the patch that pulls common code to single
> >>>place, so that single place needs to be patched. Plus I guess I should
> >>>add ifdefs, so that more advanced NAPI / tx coalescing code can be
> >>>reactivated when it is fixed. Trivial fixes can go on top. Does that
> >>>sound like a plan?
> >>
> >>Hmm, what I find strange is that, just this code is running since a
> >>long time on several platforms and Chip versions. No raise condition
> >>have been found or lock protection problems (also proving look
> >>mechanisms).
> >
> >Well, it works better for me when I disable CONFIG_SMP. It is normal
> >that locking problems are hard to reproduce :-(.
>
> can you share me the test, maybe I can try to reproduce on ARM box.
> Are you using 3.x or 4.x GMAC?
I'm using board similar to altera socfpga. 3.70a, as far as I can
tell.
gmac0: ethernet@ff70 {
compatible = "altr,socfpga-stmmac", "snps,dwmac-3.70a",
"snps,dwmac\
";
> >>Pavel, I ask you sorry if I missed some problems so, if you can
> >>(as D. Miller asked) to send us a cover letter + all patches
> >>I will try to reply soon. I can do also some tests if you ask
> >>me that! I could run on 3.x and 4.x but I cannot promise you
> >>benchmarks.
> >
> >Actually... I have questions here. David normally pulls from you (can
> >I have a address of your git tree?).
>
> No I send the patches to the mailing list.
Aha, ok.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: stmmac: turn coalescing / NAPI off in stmmac
Hi Pavel On 12/2/2016 11:42 AM, Pavel Machek wrote: Hi! Anyway... since you asked. I belive I have way to disable NAPI / tx coalescing in the driver. Unfortunately, locking is missing on the rx path, and needs to be extended to _irqsave variant on tx path. I have just replied to a previous thread about that... Yeah, please reply to David's mail where he describes why it can't work. let me to re-check the mails :-) I can try to provide you more details about what I experimented So patch currently looks like this (hand edited, can't be applied, got it working few hours ago). Does it look acceptable? I'd prefer this to go after the patch that pulls common code to single place, so that single place needs to be patched. Plus I guess I should add ifdefs, so that more advanced NAPI / tx coalescing code can be reactivated when it is fixed. Trivial fixes can go on top. Does that sound like a plan? Hmm, what I find strange is that, just this code is running since a long time on several platforms and Chip versions. No raise condition have been found or lock protection problems (also proving look mechanisms). Well, it works better for me when I disable CONFIG_SMP. It is normal that locking problems are hard to reproduce :-(. can you share me the test, maybe I can try to reproduce on ARM box. Are you using 3.x or 4.x GMAC? Pavel, I ask you sorry if I missed some problems so, if you can (as D. Miller asked) to send us a cover letter + all patches I will try to reply soon. I can do also some tests if you ask me that! I could run on 3.x and 4.x but I cannot promise you benchmarks. Actually... I have questions here. David normally pulls from you (can I have a address of your git tree?). No I send the patches to the mailing list. Could you apply these to your git? [PATCH] stmmac ethernet: unify locking [PATCH] stmmac: simplify flag assignment [PATCH] stmmac: cleanup documenation, make it match reality They are rather trivial and independend, I'm not sure what cover letter would say, besides "simple fixes". Then I can re-do the reset on top of that... Which tree do you want patches against? https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/ ? I think that bug fixing should be on top of net.git but I let Miller to decide. Hmm. It is "only" a performance problem (40msec delays).. I guess -next is better target. ok, maybe if you resend these with a cover-letter I can try to contribute on reviewing (in case of I have missed some detail). Best Regards Peppe Best regards, Pavel
Re: stmmac: turn coalescing / NAPI off in stmmac
Hi! > >Anyway... since you asked. I belive I have way to disable NAPI / tx > >coalescing in the driver. Unfortunately, locking is missing on the rx > >path, and needs to be extended to _irqsave variant on tx path. > > I have just replied to a previous thread about that... Yeah, please reply to David's mail where he describes why it can't work. > >So patch currently looks like this (hand edited, can't be > >applied, got it working few hours ago). Does it look acceptable? > > > >I'd prefer this to go after the patch that pulls common code to single > >place, so that single place needs to be patched. Plus I guess I should > >add ifdefs, so that more advanced NAPI / tx coalescing code can be > >reactivated when it is fixed. Trivial fixes can go on top. Does that > >sound like a plan? > > Hmm, what I find strange is that, just this code is running since a > long time on several platforms and Chip versions. No raise condition > have been found or lock protection problems (also proving look > mechanisms). Well, it works better for me when I disable CONFIG_SMP. It is normal that locking problems are hard to reproduce :-(. > Pavel, I ask you sorry if I missed some problems so, if you can > (as D. Miller asked) to send us a cover letter + all patches > I will try to reply soon. I can do also some tests if you ask > me that! I could run on 3.x and 4.x but I cannot promise you > benchmarks. Actually... I have questions here. David normally pulls from you (can I have a address of your git tree?). Could you apply these to your git? [PATCH] stmmac ethernet: unify locking [PATCH] stmmac: simplify flag assignment [PATCH] stmmac: cleanup documenation, make it match reality They are rather trivial and independend, I'm not sure what cover letter would say, besides "simple fixes". Then I can re-do the reset on top of that... > >Which tree do you want patches against? > > > >https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/ ? > > I think that bug fixing should be on top of net.git but I let Miller > to decide. Hmm. It is "only" a performance problem (40msec delays).. I guess -next is better target. Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: stmmac: turn coalescing / NAPI off in stmmac
On 12/1/2016 11:48 PM, Pavel Machek wrote:
@@ -2771,12 +2771,8 @@ static netdev_features_t stmmac_fix_features(struct
net_device *dev,
features &= ~NETIF_F_CSUM_MASK;
/* Disable tso if asked by ethtool */
- if ((priv->plat->tso_en) && (priv->dma_cap.tsoen)) {
- if (features & NETIF_F_TSO)
- priv->tso = true;
- else
- priv->tso = false;
- }
+ if ((priv->plat->tso_en) && (priv->dma_cap.tsoen))
+ priv->tso = !!(features & NETIF_F_TSO);
Pavel, this really seems arbitrary.
Whilst I really appreciate you're looking into this driver a bit because
of some issues you are trying to resolve, I'd like to ask that you not
start bombarding me with nit-pick cleanups here and there and instead
concentrate on the real bug or issue.
Well, fixing clean code is easier than fixing strange code... Plus I
was hoping to make the mainainers to talk. The driver is listed as
supported after all.
Absolutely, I am available to support you, better I can.
So no problem to clarify strange or complex parts of the driver
and find/try new solutions to enhance it.
Anyway... since you asked. I belive I have way to disable NAPI / tx
coalescing in the driver. Unfortunately, locking is missing on the rx
path, and needs to be extended to _irqsave variant on tx path.
I have just replied to a previous thread about that...
To be honest, I have in the box just a patch to fix lock on lpi
as I had discussed in this mailing list some week ago.
I will provide it asap.
So patch currently looks like this (hand edited, can't be
applied, got it working few hours ago). Does it look acceptable?
I'd prefer this to go after the patch that pulls common code to single
place, so that single place needs to be patched. Plus I guess I should
add ifdefs, so that more advanced NAPI / tx coalescing code can be
reactivated when it is fixed. Trivial fixes can go on top. Does that
sound like a plan?
Hmm, what I find strange is that, just this code is running since a
long time on several platforms and Chip versions. No raise condition
have been found or lock protection problems (also proving look
mechanisms).
I'd like to avoid to break old compatibilities and having the same
performances but if there are some bugs I can support to review
and test. Indeed, this year we have added the 4.x but some parts
of the code (for TSO) should be self-contained. So I cannot image
regressions on common part of the code... I let Alex to do a double
check.
Pavel, I ask you sorry if I missed some problems so, if you can
(as D. Miller asked) to send us a cover letter + all patches
I will try to reply soon. I can do also some tests if you ask
me that! I could run on 3.x and 4.x but I cannot promise you
benchmarks.
Which tree do you want patches against?
https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/ ?
I think that bug fixing should be on top of net.git but I let Miller
to decide.
Best Regards
Peppe
Best regards,
Pavel
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0b706a7..c0016c8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1395,9 +1397,10 @@ static void __stmmac_tx_clean(struct stmmac_priv *priv)
static void stmmac_tx_clean(struct stmmac_priv *priv)
{
- spin_lock(&priv->tx_lock);
+ unsigned long flags;
+ spin_lock_irqsave(&priv->tx_lock, flags);
__stmmac_tx_clean(priv);
- spin_unlock(&priv->tx_lock);
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
}
static inline void stmmac_enable_dma_irq(struct stmmac_priv *priv)
@@ -1441,6 +1444,8 @@ static void stmmac_tx_err(struct stmmac_priv *priv)
netif_wake_queue(priv->dev);
}
+static int stmmac_rx(struct stmmac_priv *priv, int limit);
+
/**
* stmmac_dma_interrupt - DMA ISR
* @priv: driver private structure
@@ -1452,10 +1457,17 @@ static void stmmac_dma_interrupt(struct stmmac_priv
*priv)
{
int status;
int rxfifosz = priv->plat->rx_fifo_size;
+ unsigned long flags;
status = priv->hw->dma->dma_interrupt(priv->ioaddr, &priv->xstats);
if (likely((status & handle_rx)) || (status & handle_tx)) {
+ int r;
+ spin_lock_irqsave(&priv->tx_lock, flags);
+ r = stmmac_rx(priv, 999);
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+#if 0
if (likely(napi_schedule_prep(&priv->napi))) {
//pr_err("napi: schedule\n");
stmmac_disable_dma_irq(priv);
@@ -1463,7 +1475,8 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
} else
pr_err("napi: schedule failed\n");
#endif
+ stmmac_tx
stmmac: turn coalescing / NAPI off in stmmac
> > @@ -2771,12 +2771,8 @@ static netdev_features_t stmmac_fix_features(struct
> > net_device *dev,
> > features &= ~NETIF_F_CSUM_MASK;
> >
> > /* Disable tso if asked by ethtool */
> > - if ((priv->plat->tso_en) && (priv->dma_cap.tsoen)) {
> > - if (features & NETIF_F_TSO)
> > - priv->tso = true;
> > - else
> > - priv->tso = false;
> > - }
> > + if ((priv->plat->tso_en) && (priv->dma_cap.tsoen))
> > + priv->tso = !!(features & NETIF_F_TSO);
> >
>
> Pavel, this really seems arbitrary.
>
> Whilst I really appreciate you're looking into this driver a bit because
> of some issues you are trying to resolve, I'd like to ask that you not
> start bombarding me with nit-pick cleanups here and there and instead
> concentrate on the real bug or issue.
Well, fixing clean code is easier than fixing strange code... Plus I
was hoping to make the mainainers to talk. The driver is listed as
supported after all.
Anyway... since you asked. I belive I have way to disable NAPI / tx
coalescing in the driver. Unfortunately, locking is missing on the rx
path, and needs to be extended to _irqsave variant on tx path.
So patch currently looks like this (hand edited, can't be
applied, got it working few hours ago). Does it look acceptable?
I'd prefer this to go after the patch that pulls common code to single
place, so that single place needs to be patched. Plus I guess I should
add ifdefs, so that more advanced NAPI / tx coalescing code can be
reactivated when it is fixed. Trivial fixes can go on top. Does that
sound like a plan?
Which tree do you want patches against?
https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/ ?
Best regards,
Pavel
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 0b706a7..c0016c8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1395,9 +1397,10 @@ static void __stmmac_tx_clean(struct stmmac_priv *priv)
static void stmmac_tx_clean(struct stmmac_priv *priv)
{
- spin_lock(&priv->tx_lock);
+ unsigned long flags;
+ spin_lock_irqsave(&priv->tx_lock, flags);
__stmmac_tx_clean(priv);
- spin_unlock(&priv->tx_lock);
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
}
static inline void stmmac_enable_dma_irq(struct stmmac_priv *priv)
@@ -1441,6 +1444,8 @@ static void stmmac_tx_err(struct stmmac_priv *priv)
netif_wake_queue(priv->dev);
}
+static int stmmac_rx(struct stmmac_priv *priv, int limit);
+
/**
* stmmac_dma_interrupt - DMA ISR
* @priv: driver private structure
@@ -1452,10 +1457,17 @@ static void stmmac_dma_interrupt(struct stmmac_priv
*priv)
{
int status;
int rxfifosz = priv->plat->rx_fifo_size;
+ unsigned long flags;
status = priv->hw->dma->dma_interrupt(priv->ioaddr, &priv->xstats);
if (likely((status & handle_rx)) || (status & handle_tx)) {
+ int r;
+ spin_lock_irqsave(&priv->tx_lock, flags);
+ r = stmmac_rx(priv, 999);
+ spin_unlock_irqrestore(&priv->tx_lock, flags);
+#if 0
if (likely(napi_schedule_prep(&priv->napi))) {
//pr_err("napi: schedule\n");
stmmac_disable_dma_irq(priv);
@@ -1463,7 +1475,8 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
} else
pr_err("napi: schedule failed\n");
#endif
+ stmmac_tx_clean(priv);
}
if (unlikely(status & tx_hard_error_bump_tc)) {
/* Try to bump up the dma threshold on this failure */
@@ -1638,7 +1651,7 @@ static void stmmac_tx_timer(unsigned long data)
{
struct stmmac_priv *priv = (struct stmmac_priv *)data;
- stmmac_tx_clean(priv);
+ //stmmac_tx_clean(priv);
}
/**
@@ -1990,6 +2003,8 @@ static void stmmac_xmit_common(struct sk_buff *skb,
struct net_device *dev, int
if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
mod_timer(&priv->txtimer,
STMMAC_COAL_TIMER(priv->tx_coal_timer));
+ priv->hw->desc->set_tx_ic(desc);
+ priv->xstats.tx_set_ic_bit++;
} else {
priv->tx_count_frames = 0;
priv->hw->desc->set_tx_ic(desc);
@@ -2038,8 +2053,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb,
struct net_device *dev)
struct dma_desc *desc, *first, *mss_desc = NULL;
u8 proto_hdr_len;
int i;
+ unsigned long flags;
- spin_lock(&priv->tx_lock);
+ spin_lock_irqsave(&priv->tx_lock, flags);
/* Compute header lengths */
proto_hdr_len = skb_transport_offset(skb) + tcp_hdrlen(skb);
@@ -2052,7 +2068,7 @@ static ne
