Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-25 Thread Helmut Schaa 
On Wed, Jan 25, 2012 at 6:08 PM, Roman Yeryomin  wrote:
> On 23 January 2012 16:26, Helmut Schaa  wrote:
>>> Well, ok, I'll retest everything again (a bit later) and let you know
>>> the results.
>>
>> Ok, thanks.
>
> It could be that you were right that my testing environment/hardware
> is not good enough.
> When I started to use iperf wireless card (well, probably the driver)
> in my laptop behaved very bad. It hanged pretty frequently (~20% of
> the time) and speed tests were very weird - sometimes I was getting
> stable 65 Mbit/s but sometimes it was jumping from 20 to 65 per test.
> Device placement and environment was the same all the time.
> The thing is that I have to use proprietary broadcom driver for this
> card (BCM4321) to get n mode. And it's very unstable.
> So, for now, I can't say anything useful.
> Probably I should get some good and supported 802.11n hardware. Any
> advises for miniPCI and/or miniPCI express cards? Atheros based?
> Ralink?

I usually use Intel devices (5100 or 5300) as clients for my tests.
However, be aware that wifi tests are never really accurate due to the
nature of the wireless media. And also the rate control interaction with
rt2x00 is not yet perfect, this might be why you sometimes see
these big throughput differences. I've got a few more patches to
improve that but they are not ready for upstream proposal yet.

Thanks for testing again!

Helmut
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-25 Thread Roman Yeryomin 
On 23 January 2012 16:26, Helmut Schaa  wrote:
>> Well, ok, I'll retest everything again (a bit later) and let you know
>> the results.
>
> Ok, thanks.

It could be that you were right that my testing environment/hardware
is not good enough.
When I started to use iperf wireless card (well, probably the driver)
in my laptop behaved very bad. It hanged pretty frequently (~20% of
the time) and speed tests were very weird - sometimes I was getting
stable 65 Mbit/s but sometimes it was jumping from 20 to 65 per test.
Device placement and environment was the same all the time.
The thing is that I have to use proprietary broadcom driver for this
card (BCM4321) to get n mode. And it's very unstable.
So, for now, I can't say anything useful.
Probably I should get some good and supported 802.11n hardware. Any
advises for miniPCI and/or miniPCI express cards? Atheros based?
Ralink?

Regards,
Roman
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-23 Thread Roman Yeryomin 
On 23 January 2012 16:26, Helmut Schaa  wrote:
> On Mon, Jan 23, 2012 at 3:13 PM, Roman Yeryomin  wrote:
>> On 23 January 2012 14:38, Helmut Schaa  wrote:
>>> On Mon, Jan 23, 2012 at 11:37 AM, Roman Yeryomin  
>>> wrote:
 I will test more thoroughly but I noticed about 0.7 MB/s speed drop
 comparing with those spinlocks commented out.
>>>
>>> Hmm, ok, might as well just be a wifi interference/noise issue? Maybe just
>>> try on eth only with iperf or something similar? With a dedicated peer and
>>> a simple crossover cable maybe.
>>
>> I don't think it's interference/noise issue as I've tried several
>> times with and without spinlocks before writing to the list.
>> The interesting part was that on eth only (bridged or routed) the
>> speed was the same (...I think so, I'm not sure now) with and without
>> spinlocks - 11.2 MB/s.
>
> Maybe because the CPU wasn't exhausted with "only" ethernet throughput
> while adding bridge + wifi will bring the CPU further to its limits
> and ksoftirqd
> kicks in ...

I guess bridge is always there (with br-lan)?

> And you really just removed the spin_lock in the housekeeping tasklet
> and nothing else? So, you haven't reverted the whole patch, right?

Yes, I just commented out those two spinlock lines.

Regards,
Roman
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-23 Thread Helmut Schaa 
On Mon, Jan 23, 2012 at 3:13 PM, Roman Yeryomin  wrote:
> On 23 January 2012 14:38, Helmut Schaa  wrote:
>> On Mon, Jan 23, 2012 at 11:37 AM, Roman Yeryomin  
>> wrote:
>>> I will test more thoroughly but I noticed about 0.7 MB/s speed drop
>>> comparing with those spinlocks commented out.
>>
>> Hmm, ok, might as well just be a wifi interference/noise issue? Maybe just
>> try on eth only with iperf or something similar? With a dedicated peer and
>> a simple crossover cable maybe.
>
> I don't think it's interference/noise issue as I've tried several
> times with and without spinlocks before writing to the list.
> The interesting part was that on eth only (bridged or routed) the
> speed was the same (...I think so, I'm not sure now) with and without
> spinlocks - 11.2 MB/s.

Maybe because the CPU wasn't exhausted with "only" ethernet throughput
while adding bridge + wifi will bring the CPU further to its limits
and ksoftirqd
kicks in ...

And you really just removed the spin_lock in the housekeeping tasklet
and nothing else? So, you haven't reverted the whole patch, right?

> Well, ok, I'll retest everything again (a bit later) and let you know
> the results.

Ok, thanks.

Helmut
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-23 Thread Roman Yeryomin 
On 23 January 2012 14:38, Helmut Schaa  wrote:
> On Mon, Jan 23, 2012 at 11:37 AM, Roman Yeryomin  
> wrote:
>> I will test more thoroughly but I noticed about 0.7 MB/s speed drop
>> comparing with those spinlocks commented out.
>
> Hmm, ok, might as well just be a wifi interference/noise issue? Maybe just
> try on eth only with iperf or something similar? With a dedicated peer and
> a simple crossover cable maybe.

I don't think it's interference/noise issue as I've tried several
times with and without spinlocks before writing to the list.
The interesting part was that on eth only (bridged or routed) the
speed was the same (...I think so, I'm not sure now) with and without
spinlocks - 11.2 MB/s.
Well, ok, I'll retest everything again (a bit later) and let you know
the results.

Regards,
Roman
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-23 Thread Helmut Schaa 
On Mon, Jan 23, 2012 at 11:37 AM, Roman Yeryomin  wrote:
> I will test more thoroughly but I noticed about 0.7 MB/s speed drop
> comparing with those spinlocks commented out.

Hmm, ok, might as well just be a wifi interference/noise issue? Maybe just
try on eth only with iperf or something similar? With a dedicated peer and
a simple crossover cable maybe.

 Are you sure it's needed?
>>
>> No, but the spinlock_irqsave locked between the housekeeping path
>> and the tx path. The new locking just does the same without the need to
>> disabled IRQs.
>
> Yes, but houskeeping tasklet is also called from other places too.

Correct. The tx path used spin_lock_irqsave on page_lock, which is _only_
used in the xmit function
=> Hence, it basically downgrades the spin_lock_irqsave to a
local_irq_disable which disables hard IRQs. And prevents the "normal"
housekeeping in IRQ context to run.

The ramips_eth_timeout is unlikely to be called.

And furthermore, we're on a UP machine. There's only one tasklet run at
once. Hence, no locking is required (sort of). That's also why the spin_lock
should be optimized out in that case.

> And as I understand interrupts are disabled before calling tasklet anyway?

No. It wouldn't make sense to disable hard IRQs while a tasklet is run. If this
would be the case you could just put everything in the IRQ handler.

> I can't say much about what is right and what is wrong but reading
> about tasklets 
> (http://home.comcast.net/~heidi.young1/projects/cs517b/linuxkernel_ch6/tasklets.htm)
> I noticed that before scheduling a tasklet irq state have to be saved
> (I suppose that is what irqsave is for)?
> Sorry for the dumb questions if they are.

tasklet_schedule will disable IRQs while adding the tasklet to the list
of scheduled tasklets as otherwise another IRQ could interfere
while manipulating the list of scheduled tasklets.

While the tasklets are executed hard IRQs are most of the time
enabled.

Helmut
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-23 Thread Roman Yeryomin 
On 23 January 2012 11:27, Helmut Schaa  wrote:
> On Sun, Jan 22, 2012 at 11:44 AM, John Crispin  wrote:
>> On 22/01/12 02:45, Roman Yeryomin wrote:
>>> On 17 January 2012 11:42, Helmut Schaa  wrote:
 @@ -313,6 +312,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
        struct net_device *dev = (struct net_device*)ptr;
        struct raeth_priv *priv = netdev_priv(dev);

 +       spin_lock(&priv->page_lock);
        while ((priv->tx[priv->skb_free_idx].txd2 & TX_DMA_DONE) &&
               (priv->tx_skb[priv->skb_free_idx])) {
                dev_kfree_skb_irq(priv->tx_skb[priv->skb_free_idx]);
 @@ -321,6 +321,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
                if (priv->skb_free_idx >= NUM_TX_DESC)
                        priv->skb_free_idx = 0;
        }
 +       spin_unlock(&priv->page_lock);

        ramips_fe_int_enable(RAMIPS_TX_DLY_INT);
  }
>>>
>>> Seems that spinlock here introduces performance hit in wifi<->eth path.
>
> You're sure? I can still get 60Mbit/s TCP bridged between eth <->wifi and
> on UP (like all ramips devices) systems the spinlock should be optimized
> out anyway. The only overhead that gets added is that housekeeping is
> done in softirq context instead of hardirq but that is justified as we should
> only do _minimal_ work in the actual IRQ handler ...

I will test more thoroughly but I noticed about 0.7 MB/s speed drop
comparing with those spinlocks commented out.

>>> Are you sure it's needed?
>
> No, but the spinlock_irqsave locked between the housekeeping path
> and the tx path. The new locking just does the same without the need to
> disabled IRQs.

Yes, but houskeeping tasklet is also called from other places too.
And as I understand interrupts are disabled before calling tasklet anyway?
I can't say much about what is right and what is wrong but reading
about tasklets 
(http://home.comcast.net/~heidi.young1/projects/cs517b/linuxkernel_ch6/tasklets.htm)
I noticed that before scheduling a tasklet irq state have to be saved
(I suppose that is what irqsave is for)?
Sorry for the dumb questions if they are.


Regards,
Roman
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-23 Thread Helmut Schaa 
On Sun, Jan 22, 2012 at 11:44 AM, John Crispin  wrote:
> On 22/01/12 02:45, Roman Yeryomin wrote:
>> On 17 January 2012 11:42, Helmut Schaa  wrote:
>>> @@ -313,6 +312,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
>>>        struct net_device *dev = (struct net_device*)ptr;
>>>        struct raeth_priv *priv = netdev_priv(dev);
>>>
>>> +       spin_lock(&priv->page_lock);
>>>        while ((priv->tx[priv->skb_free_idx].txd2 & TX_DMA_DONE) &&
>>>               (priv->tx_skb[priv->skb_free_idx])) {
>>>                dev_kfree_skb_irq(priv->tx_skb[priv->skb_free_idx]);
>>> @@ -321,6 +321,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
>>>                if (priv->skb_free_idx >= NUM_TX_DESC)
>>>                        priv->skb_free_idx = 0;
>>>        }
>>> +       spin_unlock(&priv->page_lock);
>>>
>>>        ramips_fe_int_enable(RAMIPS_TX_DLY_INT);
>>>  }
>>
>> Seems that spinlock here introduces performance hit in wifi<->eth path.

You're sure? I can still get 60Mbit/s TCP bridged between eth <->wifi and
on UP (like all ramips devices) systems the spinlock should be optimized
out anyway. The only overhead that gets added is that housekeeping is
done in softirq context instead of hardirq but that is justified as we should
only do _minimal_ work in the actual IRQ handler ...

>> Are you sure it's needed?

No, but the spinlock_irqsave locked between the housekeeping path
and the tx path. The new locking just does the same without the need to
disabled IRQs.

> Seems that it works fine also without this spinlock.

I wouldn't remove the spinlock without very good code review! If you
can explain why it is not needed I'm fine with removing it but I haven't
done an exhausting review yet, so this might take some time ...

Helmut
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-22 Thread John Crispin 
On 22/01/12 02:45, Roman Yeryomin wrote:
> On 17 January 2012 11:42, Helmut Schaa  wrote:
>> @@ -313,6 +312,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
>>struct net_device *dev = (struct net_device*)ptr;
>>struct raeth_priv *priv = netdev_priv(dev);
>>
>> +   spin_lock(&priv->page_lock);
>>while ((priv->tx[priv->skb_free_idx].txd2 & TX_DMA_DONE) &&
>>   (priv->tx_skb[priv->skb_free_idx])) {
>>dev_kfree_skb_irq(priv->tx_skb[priv->skb_free_idx]);
>> @@ -321,6 +321,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
>>if (priv->skb_free_idx >= NUM_TX_DESC)
>>priv->skb_free_idx = 0;
>>}
>> +   spin_unlock(&priv->page_lock);
>>
>>ramips_fe_int_enable(RAMIPS_TX_DLY_INT);
>>  }
> 
> Seems that spinlock here introduces performance hit in wifi<->eth path.
> Are you sure it's needed? Seems that it works fine also without this spinlock.


helmut,

can you verify this ?
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-21 Thread Roman Yeryomin 
On 17 January 2012 11:42, Helmut Schaa  wrote:
> @@ -313,6 +312,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
>        struct net_device *dev = (struct net_device*)ptr;
>        struct raeth_priv *priv = netdev_priv(dev);
>
> +       spin_lock(&priv->page_lock);
>        while ((priv->tx[priv->skb_free_idx].txd2 & TX_DMA_DONE) &&
>               (priv->tx_skb[priv->skb_free_idx])) {
>                dev_kfree_skb_irq(priv->tx_skb[priv->skb_free_idx]);
> @@ -321,6 +321,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
>                if (priv->skb_free_idx >= NUM_TX_DESC)
>                        priv->skb_free_idx = 0;
>        }
> +       spin_unlock(&priv->page_lock);
>
>        ramips_fe_int_enable(RAMIPS_TX_DLY_INT);
>  }

Seems that spinlock here introduces performance hit in wifi<->eth path.
Are you sure it's needed? Seems that it works fine also without this spinlock.

Regards,
Roman
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-17 Thread John Crispin 
Thx, applied in r29762

___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[OpenWrt-Devel] [PATCH] ramips: Rework ramips_eth to not require irqsave locking anymore

2012-01-17 Thread Helmut Schaa 
Previously the tx housekeeping was done in a spin_lock_irqsave critical
section which causes irqs to be disabled during that time. Since the
housekeeping is already prepared to be scheduled as a tasklet process
the housekeeping only in softirq context and revise the locking between
the tx path and the housekeeping tasklet by using a normal spin_lock
which in most situations will be a NOP anyway.

This makes sure that interrupts are only disabled for a short time
since in the worst case the housekeeping might have to free up to 256
skbs.

Signed-off-by: Helmut Schaa 
---
 target/linux/ramips/files/drivers/net/ramips.c |   15 +--
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/target/linux/ramips/files/drivers/net/ramips.c 
b/target/linux/ramips/files/drivers/net/ramips.c
index 63570c7..0bc6c06 100644
--- a/target/linux/ramips/files/drivers/net/ramips.c
+++ b/target/linux/ramips/files/drivers/net/ramips.c
@@ -215,7 +215,6 @@ ramips_eth_hard_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
unsigned long tx;
unsigned int tx_next;
unsigned int mapped_addr;
-   unsigned long flags;
 
if (priv->plat->min_pkt_len) {
if (skb->len < priv->plat->min_pkt_len) {
@@ -233,7 +232,7 @@ ramips_eth_hard_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
mapped_addr = (unsigned int) dma_map_single(NULL, skb->data, skb->len,
DMA_TO_DEVICE);
dma_sync_single_for_device(NULL, mapped_addr, skb->len, DMA_TO_DEVICE);
-   spin_lock_irqsave(&priv->page_lock, flags);
+   spin_lock(&priv->page_lock);
tx = ramips_fe_rr(RAMIPS_TX_CTX_IDX0);
tx_next = (tx + 1) % NUM_TX_DESC;
 
@@ -250,11 +249,11 @@ ramips_eth_hard_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
priv->tx_skb[tx] = skb;
wmb();
ramips_fe_wr(tx_next, RAMIPS_TX_CTX_IDX0);
-   spin_unlock_irqrestore(&priv->page_lock, flags);
+   spin_unlock(&priv->page_lock);
return NETDEV_TX_OK;
 
  out:
-   spin_unlock_irqrestore(&priv->page_lock, flags);
+   spin_unlock(&priv->page_lock);
dev->stats.tx_dropped++;
kfree_skb(skb);
return NETDEV_TX_OK;
@@ -313,6 +312,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
struct net_device *dev = (struct net_device*)ptr;
struct raeth_priv *priv = netdev_priv(dev);
 
+   spin_lock(&priv->page_lock);
while ((priv->tx[priv->skb_free_idx].txd2 & TX_DMA_DONE) &&
   (priv->tx_skb[priv->skb_free_idx])) {
dev_kfree_skb_irq(priv->tx_skb[priv->skb_free_idx]);
@@ -321,6 +321,7 @@ ramips_eth_tx_housekeeping(unsigned long ptr)
if (priv->skb_free_idx >= NUM_TX_DESC)
priv->skb_free_idx = 0;
}
+   spin_unlock(&priv->page_lock);
 
ramips_fe_int_enable(RAMIPS_TX_DLY_INT);
 }
@@ -346,8 +347,10 @@ ramips_eth_irq(int irq, void *dev)
tasklet_schedule(&priv->rx_tasklet);
}
 
-   if (fe_int & RAMIPS_TX_DLY_INT)
-   ramips_eth_tx_housekeeping((unsigned long)dev);
+   if (fe_int & RAMIPS_TX_DLY_INT) {
+   ramips_fe_int_disable(RAMIPS_TX_DLY_INT);
+   tasklet_schedule(&priv->tx_housekeeping_tasklet);
+   }
 
return IRQ_HANDLED;
 }
-- 
1.7.7

___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel