Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Hi Shuyu, On 05.06.2016 16:02, Shuyu Wei wrote: > Hi Lino, > Sorry for my late reply. I retested the previous patch, it did have > the same issue. However it seems that the possibility of stuck is lower > (just my instinct, no evidence). > Thank you for the feedback. It is hard to guess what

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On Mon, May 30, 2016 at 11:41:22PM +0200, Lino Sanfilippo wrote: > > Did you see the same issues with the patch before (the one that, as you wrote, > survived a whole night of stress testing)? > > Lino Hi Lino, Sorry for my late reply. I retested the previous patch, it did have the same issue.

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Hi Shuyu, On 28.05.2016 08:43, Shuyu Wei wrote: > > After some stress testing, it worked well most of the time. > But there is a chance that it may get stuck when I use 2 nc process > to send TCP packects at full speed. Only when a new rx packet > arrive can trigger it to run again. This

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On Wed, May 25, 2016 at 01:56:20AM +0200, Lino Sanfilippo wrote: > Francois, Shuyu, > > this is the patch with the discussed changes. > > Shuyu it would be great if you could test this one. If it passes > and there are no further objections I will resend it as a regular patch > (including commit

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Francois, Shuyu, this is the patch with the discussed changes. Shuyu it would be great if you could test this one. If it passes and there are no further objections I will resend it as a regular patch (including commit message, etc.) to the mailing list. diff --git

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Lino Sanfilippo : [...] > I dont agree here. A dma_wmb would have an effect to "data" and "info", yes, > but it would have absolutely no effect to skb_tx_timestamp(), since there > is no dma access involved here. In fact skb_tx_timestamp() could probably > be even reordered

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On 23.05.2016 13:36, Shuyu Wei wrote: > On Sun, May 22, 2016 at 01:30:27PM +0200, Lino Sanfilippo wrote: >> > > Hi Lino, > This patch worked after a whole night of stress testing. > Thats great! I will nevertheless make the changes discussed with Francois and hopefully we have a final

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On 23.05.2016 00:36, Francois Romieu wrote: > Lino Sanfilippo : > [...] >> --- a/drivers/net/ethernet/arc/emac_main.c >> +++ b/drivers/net/ethernet/arc/emac_main.c >> @@ -159,12 +159,22 @@ static void arc_emac_tx_clean(struct net_device *ndev) >> unsigned int

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On Sun, May 22, 2016 at 01:30:27PM +0200, Lino Sanfilippo wrote: > > Thanks for testing. However that extra check for skb not being NULL should > not be > necessary if the code were correct. The changes I suggested were all about > having > skb and info consistent with txbd_curr. > But I just

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Lino Sanfilippo : [...] > --- a/drivers/net/ethernet/arc/emac_main.c > +++ b/drivers/net/ethernet/arc/emac_main.c > @@ -159,12 +159,22 @@ static void arc_emac_tx_clean(struct net_device *ndev) > unsigned int *txbd_dirty = >txbd_dirty; > struct

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Lino Sanfilippo : > On 21.05.2016 21:47, Francois Romieu wrote: > > Shuyu Wei : > > [...] > >> diff --git a/drivers/net/ethernet/arc/emac_main.c > >> b/drivers/net/ethernet/arc/emac_main.c > >> index a3a9392..c2447b0 100644 > >> ---

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On 22.05.2016 11:17, Shuyu Wei wrote: > Hi Lino, > > I tested this patch, it still got panic under stress. > Just wget 2 large files simultaneously and it failed. > > Looks like the problem comes from the if statement in tx_clean(). > I changed your patch to use > > - if (info &

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On Sun, May 22, 2016 at 12:58:55AM +0200, Lino Sanfilippo wrote: > --- a/drivers/net/ethernet/arc/emac_main.c > +++ b/drivers/net/ethernet/arc/emac_main.c > @@ -162,7 +162,13 @@ static void arc_emac_tx_clean(struct net_device *ndev) > struct sk_buff *skb = tx_buff->skb; >

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On 21.05.2016 21:47, Francois Romieu wrote: > Shuyu Wei : > [...] >> diff --git a/drivers/net/ethernet/arc/emac_main.c >> b/drivers/net/ethernet/arc/emac_main.c >> index a3a9392..c2447b0 100644 >> --- a/drivers/net/ethernet/arc/emac_main.c >> +++

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On 21.05.2016 18:09, Shuyu Wei wrote: > Looks like I got it wrong in the first place. > > priv->tx_buff is not for the device, so there's no need to move it. > The race has been fixed by commit c278c253f3d9, I forgot to check > it out. That's my fault. > > I do find another problem. We need to

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Shuyu Wei : [...] > diff --git a/drivers/net/ethernet/arc/emac_main.c > b/drivers/net/ethernet/arc/emac_main.c > index a3a9392..c2447b0 100644 > --- a/drivers/net/ethernet/arc/emac_main.c > +++ b/drivers/net/ethernet/arc/emac_main.c > @@ -686,6 +686,9 @@ static int

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Looks like I got it wrong in the first place. priv->tx_buff is not for the device, so there's no need to move it. The race has been fixed by commit c278c253f3d9, I forgot to check it out. That's my fault. I do find another problem. We need to use a barrier to make sure skb_tx_timestamp() is

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On Thu, May 19, 2016 at 11:15:56PM +0200, Lino Sanfilippo wrote: > drivers/net/ethernet/arc/emac_main.c | 33 + > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/ethernet/arc/emac_main.c > b/drivers/net/ethernet/arc/emac_main.c >

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Lino Sanfilippo : [...] > 2. requires a smp_wmb, while 3. requires a rmb(). AFAIK the mb() implies all > we need, > the dma_rmb() for 1., the smp_wmb() for 2. and the rmb() for 3. A revalidation of tx_dirty is still needed (see below) despite the rmb() for 3. The rmb()

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Hi Francois, On 19.05.2016 00:55, Francois Romieu wrote: >> The smp_wmb() in tx function combined with the smp_rmb() in tx_clean ensures >> that the CPU running tx_clean sees consistent values for info, data and skb >> (thus no need to check for validity of all three values any more). >> The

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Lino Sanfilippo : [...] > what about the (only compile tested) code below? I may have misunderstood some parts but it nonetheless seems broken. > The smp_wmb() in tx function combined with the smp_rmb() in tx_clean ensures > that the CPU running tx_clean sees consistent

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

On 18.05.2016 02:01, Francois Romieu wrote: > The smp_wmb() and wmb() could be made side-by-side once *info is > updated but I don't see the adequate idiom to improve the smp_wmb + wmb > combo. :o/ > >> And the wmb() looks like it should be a dma_wmb(). > > I see two points against it: > - it

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

David Miller : > From: Shuyu Wei > Date: Tue, 17 May 2016 23:25:20 +0800 > > > diff --git a/drivers/net/ethernet/arc/emac_main.c > > b/drivers/net/ethernet/arc/emac_main.c > > index a3a9392..df3dfef 100644 > > --- a/drivers/net/ethernet/arc/emac_main.c >

Re: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

From: Shuyu Wei Date: Tue, 17 May 2016 23:25:20 +0800 > diff --git a/drivers/net/ethernet/arc/emac_main.c > b/drivers/net/ethernet/arc/emac_main.c > index a3a9392..df3dfef 100644 > --- a/drivers/net/ethernet/arc/emac_main.c > +++ b/drivers/net/ethernet/arc/emac_main.c > @@

Aw: [PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Hi, > Von: "Shuyu Wei" > @@ -685,13 +684,15 @@ static int arc_emac_tx(struct sk_buff *skb, struct > net_device *ndev) > wmb(); > > skb_tx_timestamp(skb); > + priv->tx_buff[*txbd_curr].skb = skb; > + > + dma_wmb(); > > *info = cpu_to_le32(FOR_EMAC

[PATCH v2] ethernet:arc: Fix racing of TX ring buffer

Setting the FOR_EMAC flag should be the last step of modifying the buffer descriptor, or possible racing will occur. The loop counter i in tx_clean() is not needed, and we need to make sure it does not clear the finished txbds. Signed-off-by: Shuyu Wei --- Changes in v2: -