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.jablon...@thalesgroup.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.kai...@thalesgroup.com>
> > CC: Jan Jablonsky <jan.jablon...@thalesgroup.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.com>
> > Tested-by: Aaron Brown <aaron.f.br...@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirs...@intel.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
