netdev - Re: [PATCH 1/1] bnx2x: Tx barriers and locks

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]

Date:	Mon, 8 Mar 2010 16:38:30 +0100
From:	Stanislaw Gruszka <sgruszka@...hat.com>
To:	Michael Chan <mchan@...adcom.com>
Cc:	Vladislav Zolotarov <vladz@...adcom.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	Eilon Greenstein <eilong@...adcom.com>,
	Matthew Carlson <mcarlson@...adcom.com>
Subject: Re: [PATCH 1/1] bnx2x: Tx barriers and locks

On Tue, Mar 02, 2010 at 09:26:27AM -0800, Michael Chan wrote:
> 
> On Tue, 2010-03-02 at 08:59 -0800, Stanislaw Gruszka wrote:
> > On Tue, Mar 02, 2010 at 08:18:44AM -0800, Michael Chan wrote:
> > > Stanislaw Gruszka wrote:
> > > 
> > > > On Tue, Mar 02, 2010 at 04:50:59AM -0800, Vladislav Zolotarov wrote:
> > > > > Stanislaw barrier() is not a memory barrier - it's a
> > > > compiler barrier. I don't think removing it from
> > > > bnx2x_tx_avail() will improve anything. If u think I'm wrong,
> > > > could u, pls., provide a specific example.
> > > >
> > > > Only improvement is removing confusing code, And comment like
> > > > "Tell compiler that prod and cons can change" is even more
> > > > confusing. If you think I'm wrong, just tell as why that
> > > > barrier is needed :)
> > > 
> > > The barrier (compiler barrier at least) is required in
> > > bnx2x_tx_avail().  The status block index can be updated by DMA and
> > > the compiler doesn't know it (because it is considered wrong to
> > 
> > If you are telling status block index you mean which variable ?
> 
> The fp-> fields which can be updated by NAPI poll based on new status
> block DMA.

So, we are talking about fp->tx_bd_prod and fp->tx_bd_cons.

> > > declare the status block as volatile).  Near the end of
> > > bnx2x_start_xmit() where we call bnx2x_tx_avail() twice.  It is
> > > possible that the compiler will optimize it and not look at the
> > > status block in memory the second time.

Still think barrier() in bnx2x_tx_avail is not necessary.

First of all, in what we have now in net-next-2.6 tree all fp->tx_bd_prod
modifications and bnx2x_tx_avail() function calls are done with
__netif_tx_lock taken, so we don't need any barriers for that. I'm omitting
here bnx2x_run_loopback() as this function is called when interface
is disabled, and bnx2x_stats_update() when output of bnx2x_tx_avail()
is printed in debug mode.

Regarding fp->tx_bd_cons: situation is a bit more complicated. It is modified
outside __netif_tx_lock protection and can happen in parallel with
bnx2x_start_xmit(). However barrier() in bnx2_tx_avail do not help when
fp->tx_bd_cons is modified on bnx2x_tx_int() on other cpu, since there
is no guarantees that registers/cache will be flushed on that cpu. Even
taking into account smp_wmb() after "fp->tx_bd_cons = bd_cons", as
smp_wmb() only guarantee preserve of ordering, not flushing registers/cache
in any particular time. For the same reason changing barrier() to smb_mb()
in bnx2x_tx_avail have no sense.

I plan to split my bnx2x patch into smaller peaces and post them. Small
patches perhaps will be more clean and You would not have objections
against :)

Stanislaw
--
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