netdev - Re: [net-next 01/10] igb: reduce CPU0 latency when updating statistics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f6ca0b3c-8099-4e99-8295-3d4c2a6150d9@THSDC2IRIMBX36P.iris.infra.thales>
Date:   Tue, 22 Jan 2019 16:12:41 +0000
From:   JABLONSKY Jan <Jan.JABLONSKY@...lesgroup.com>
To:     Ido Schimmel <idosch@...sch.org>
CC:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "nhorman@...hat.com" <nhorman@...hat.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        KAINDL Bernhard <bernhard.kaindl@...lesgroup.com>
Subject: Re: [net-next 01/10] igb: reduce CPU0 latency when updating
 statistics

On Di, 2018-12-25 at 17:47 +0200, Ido Schimmel wrote:
> On Thu, Dec 20, 2018 at 12:56:25PM -0800, Jeff Kirsher wrote:
> > This change is based off of the work and suggestion of Jan Jablonsky
> > <jan.jablonsky@...lesgroup.com>.
> > 
> > The Watchdog workqueue in igb driver is scheduled every 2s for each
> > network interface. That includes updating a statistics protected by
> > spinlock. Function igb_update_stats in this case will be protected
> > against preemption. According to number of a statistics registers
> > (cca 60), processing this function might cause additional cpu load
> >  on CPU0.
> > 
> > In case of statistics spinlock may be replaced with mutex, which
> > reduce latency on CPU0.
> > 
> > CC: Bernhard Kaindl  <bernhard.kaindl@...lesgroup.com>
> > CC: Jan Jablonsky <jan.jablonsky@...lesgroup.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> > Tested-by: Aaron Brown <aaron.f.brown@...el.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> 
> Hi,
> 
> I'm hitting following issue with this commit:
> https://lists.01.org/pipermail/lkp/2018-September/009139.html

Sorry for late answer,

Thanks for input
I didnt have much time to take a look. 


Right, using mutex_lock is not the best solution for igb_get_stats64
with not activated CONFIG_DEBUG_ATOMIC_SLEEP

Anyway spinlocks dont solve latency problem (mostly when you use CONFIG_PREEMPT)


I was just consider solution to avoid "blocking" in igb_get_stats64
- Main idea is to immediately return "the last valid data" in igb_get_stats64



Experiment from today:

        struct mutex stats64_lock;
        struct rtnl_link_stats64 stats64;
        struct rtnl_link_stats64 stats64_backup;



static struct rtnl_link_stats64 *igb_get_stats64(struct net_device *netdev,
                                                struct rtnl_link_stats64 *stats)
{
        struct igb_adapter *adapter = netdev_priv(netdev);

        if (! mutex_trylock(&adapter->stats64_lock)) {
                memcpy(stats, &adapter->stats64_backup, sizeof(*stats));
                goto out;
        }

        igb_update_stats(adapter, &adapter->stats64);
        memcpy(&adapter->stats64_backup, &adapter->stats64, sizeof(*stats));
        memcpy(stats, &adapter->stats64, sizeof(*stats));
        mutex_unlock(&adapter->stats64_lock);
out:
        return stats;
}

Easy example:
while true; do ifconfig > /dev/null; done &
CNT_ETH=6; while true; do for i in `seq 0 $(($CNT_ETH - 1))`; do ethtool -S eth$i > /dev/null; done;  done &
cyclictest -SMmp 90


CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_PREEMPT=y


Default spinlock implementation:
T: 0 ( 3779) P:90 I:1000 C:  90614 Min:      2 Act:    5 Avg:    8 Max:     239
T: 1 ( 3781) P:90 I:1500 C:  60406 Min:      2 Act:    3 Avg:   10 Max:     210
T: 2 ( 3784) P:90 I:2000 C:  45303 Min:      2 Act:  213 Avg:   14 Max:     213
T: 3 ( 3787) P:90 I:2500 C:  36240 Min:      2 Act:   10 Avg:   12 Max:     221


(No BUG messages)
Modified igb_get_stats64 with mutex_trylock + backup:
T: 0 (13138) P:90 I:1000 C: 138238 Min:      2 Act:    8 Avg:    8 Max:      64
T: 1 (13140) P:90 I:1500 C:  92156 Min:      2 Act:   38 Avg:    8 Max:      38
T: 2 (13143) P:90 I:2000 C:  69115 Min:      2 Act:   11 Avg:   12 Max:      43
T: 3 (13146) P:90 I:2500 C:  55290 Min:      2 Act:   13 Avg:   11 Max:      43






But its just a experiment


Anyway mentioned patch was already reverted

Sorry for the inconveniences


Regards
Jan