netdev - Re: [PATCH (net.git)] stmmac: fix and review whole driver locking

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 04 Sep 2014 22:22:40 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	peppe.cavallaro@...com
Cc:	netdev@...r.kernel.org, bigeasy@...utronix.de,
	khoroshilov@...ras.ru
Subject: Re: [PATCH (net.git)] stmmac: fix and review whole driver locking

From: Giuseppe Cavallaro <peppe.cavallaro@...com>
Date: Tue, 2 Sep 2014 08:00:03 +0200

> The patch reviews the tx lock removing it because the driver
> claims the resource in NAPI context and, as designed, can run
> w/o any own extra lock (so just netif_tx_lock).
> This shows an impact on performances too.

It's not that simple, you have to make some changes if you really
want to allow these two threads of control to run asynchronously.

Look at this comment from tg3_tx() in the tg3 driver for example:

====================
	tnapi->tx_cons = sw_idx;

	/* Need to make the tx_cons update visible to tg3_start_xmit()
	 * before checking for netif_queue_stopped().  Without the
	 * memory barrier, there is a small possibility that tg3_start_xmit()
	 * will miss it and cause the queue to be stopped forever.
	 */
	smp_mb();

	if (unlikely(netif_tx_queue_stopped(txq) &&
====================

With the lock removed, these two code paths will operate and execute
completely in parallel with eachother.  Therefore the updates of
certain TX ring state variables will need to be strictly ordered.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html