Re: [net-next.git 3/8 (V2)] stmmac: add the initial tx coalesce schema

[Giuseppe CAVALLARO <peppe.cavallaro@st.com>]

On 9/18/2012 7:41 AM, Giuseppe CAVALLARO wrote:
> Hello David, Ben,
>
> On 9/14/2012 9:36 AM, Giuseppe CAVALLARO wrote:
>> On 9/13/2012 10:23 PM, David Miller wrote:
>>> From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
>>> Date: Tue, 11 Sep 2012 08:55:09 +0200
>>>
>>>> +    unsigned long flags;
>>>> +
>>>> +    spin_lock_irqsave(&priv->tx_lock, flags);
>>>>
>>>> -    spin_lock(&priv->tx_lock);
>>>> +    priv->xstats.tx_clean++;
>>>
>>> You are changing the locking here for the sake of the new timer.
>>>
>>> But timers run in software interrupt context, so this change is
>>> completely unnecessary since NAPI runs in software interrupt context
>>> as well, and neither timers nor NAPI run in hardware interrupts
>>> context.
>>
>> Indeed It can be called by the ISR too in this new implementation.
>> I have added the spin_lock_irqsave/restore otherwise, testing with
>> CONFIG_PROVE_LOOKING, I get the following warning on ARM SMP.
>
> sorry if I disturb you again, any news on these patches?
> Please, let me know.

Hello David,

I'd like to review these patches and resend them again (not now that 
net-next is closed) especially because they improve the driver 
performances and other people can get these benefits.

I've some doubts about what exactly I have to do. Sorry if I bother you.

Some of these patches were also reviewed by Ben long time ago; now there 
is only pending the spinlock modifications where you've had some concerns.

Maybe, I was not so clear in my comments so let me provide you further 
details and please let me know.

The stmmac_tx (to clear the tx resources) is now called by both the sw 
timer and the Interrupt handler.

   Note:
   I have also added a threshold that allows to set the interrupt on
   completion bit after a number of frames (Ben also game me some useful
   info to fix this code). On my tests, using both SH4 and ARM (SMP)
   platforms, this helped to make the driver more reactive on heavy
   traffic. Only using the timer I noticed that the driver losses frames
   in some network benchmarks (with iperf, netperf, nuttcp).

On ARM SMP, with  CONFIG_PROVE_LOOKING enabled, I got the info about the 
inconsistent lock state. Indeed, it makes sense to me to protect the 
stmmac_tx from irq in this new implementation.

What do you think, welcome your feedback.

Best Regards
Peppe

>
> Regards
> Peppe
>
>>
>> [    8.030000]
>> [    8.030000] =================================
>> [    8.030000] [ INFO: inconsistent lock state ]
>> [    8.030000] 3.4.7_stm24_0302-b2000+ #103 Not tainted
>> [    8.030000] ---------------------------------
>> [    8.030000] inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
>> [    8.030000] swapper/0/1 [HC1[1]:SC0[0]:HE0:SE1] takes:
>> [    8.030000]  (&(&priv->tx_lock)->rlock){?.-...}, at: [<802651d8>]
>> stmmac_tx+0x1c/0x388
>> [    8.030000] {HARDIRQ-ON-W} state was registered at:
>> [    8.030000]   [<800562b4>] __lock_acquire+0x638/0x179c
>> [    8.030000]   [<80057884>] lock_acquire+0x60/0x74
>> [    8.030000]   [<80428a08>] _raw_spin_lock+0x40/0x50
>> [    8.030000]   [<802651d8>] stmmac_tx+0x1c/0x388
>> [    8.030000]   [<80026be0>] run_timer_softirq+0x180/0x23c
>> [    8.030000]   [<80020ccc>] __do_softirq+0xa0/0x114
>> [    8.030000]   [<80021204>] irq_exit+0x58/0x7c
>> [    8.030000]   [<8000dc80>] handle_IRQ+0x7c/0xb8
>> [    8.030000]   [<80008464>] gic_handle_irq+0x34/0x58
>> [    8.030000]   [<80429684>] __irq_svc+0x44/0x78
>> [    8.030000]   [<8001c3f4>] vprintk+0x41c/0x480
>> [    8.030000]   [<8042097c>] printk+0x18/0x24
>> [    8.030000]   [<805aef6c>] prepare_namespace+0x1c/0x1a4
>> [    8.030000]   [<805ae980>] kernel_init+0x1c8/0x20c
>> [    8.030000]   [<8000deb8>] kernel_thread_exit+0x0/0x8
>> [    8.030000] irq event stamp: 254745
>> [    8.030000] hardirqs last  enabled at (254744): [<80429240>]
>> _raw_spin_unlock_irqrestore+0x3c/0x6c
>> [    8.030000] hardirqs last disabled at (254745): [<80429674>]
>> __irq_svc+0x34/0x78
>> [    8.030000] softirqs last  enabled at (254741): [<8035d964>]
>> dev_queue_xmit+0x6a4/0x724
>> [    8.030000] softirqs last disabled at (254737): [<8035d2d4>]
>> dev_queue_xmit+0x14/0x724
>> [    8.030000]
>> [    8.030000] other info that might help us debug this:
>> [    8.030000]  Possible unsafe locking scenario:
>> [    8.030000]
>> [    8.030000]        CPU0
>> [    8.030000]        ----
>> [    8.030000]   lock(&(&priv->tx_lock)->rlock);
>> [    8.030000]   <Interrupt>
>> [    8.030000]     lock(&(&priv->tx_lock)->rlock);
>> [    8.030000]
>> [    8.030000]  *** DEADLOCK ***
>>
>>> Therefore, disabling hardware interrupts for this lock is unnecessary
>>> and will decrease performance.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>