netdev - Re: [PATCH v2] ucc_geth: Reduce IRQ off in xmit path

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, 20 Sep 2012 17:08:52 -0400 (EDT)
From:	David Miller <davem@...emloft.net>
To:	Joakim.Tjernlund@...nsmode.se
Cc:	netdev@...r.kernel.org, romieu@...zoreil.com
Subject: Re: [PATCH v2] ucc_geth: Reduce IRQ off in xmit path

From: Joakim Tjernlund <Joakim.Tjernlund@...nsmode.se>
Date: Thu, 20 Sep 2012 10:17:01 +0200

> Currently ucc_geth_start_xmit wraps IRQ off for the
> whole body just to be safe.
> Reduce the IRQ off period to a minimum.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@...nsmode.se>
> ---
> 
>  v2: Move assignment of ugeth->tx_skbuff[txQ][ugeth->skb_curtx[txQ]]
>      inside IRQ off section to prevent racing against
>      ucc_geth_tx(). Spotted by Francois Romieu <romieu@...zoreil.com>

I agree with Francois's initial analysis, and disagree with you're
response to him, wrt. the suggest to remove all locking entirely.

Unlike what you claim, there isn't much of a gain at all from merely
make the window of lock holding smaller, especially on the scale
in which you are doing it here.

Whereas removing the lock and the atomic completely, as tg3 does,
will give very significant performance gains.

The locking cost of grabbing the spinlock, and the memory transactions
associated with it, dominate.

Furthermore, even if the gains of your change are non-trivial, you
haven't documented it.  So unless you should some noticable gains from
this, it's just code masterbation as far as I'm concerned and I'm
therefore inclined to not apply patches like this.

TG3's core interrupt locking is not that difficult to understand and
replicate in other drivers, so I dismiss your attempts to avoid that
approach on difficulty grounds as well.
--
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