netdev - Re: [RFC PATCH] bnx2x: fix tx queue locking and memory barriers

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]

Message-ID: <31355.1268240990@redhat.com>
Date:	Wed, 10 Mar 2010 17:09:50 +0000
From:	David Howells <dhowells@...hat.com>
To:	Stanislaw Gruszka <sgruszka@...hat.com>,
	Vladislav Zolotarov <vladz@...adcom.com>,
	David Miller <davem@...emloft.net>, paulmck@...ux.vnet.ibm.com
Cc:	dhowells@...hat.com, netdev@...r.kernel.org,
	Eilon Greenstein <eilong@...adcom.com>
Subject: Re: [RFC PATCH] bnx2x: fix tx queue locking and memory barriers

Stanislaw Gruszka <sgruszka@...hat.com> wrote:

> -	barrier(); /* Tell compiler that prod and cons can change */
> +	/* prod and cons can change on other cpu, want to see
> +	   consistend available space and queue (stop/running) state */
> +	smp_mb();
> +
>  	prod = fp->tx_bd_prod;
>  	cons = fp->tx_bd_cons;

I suspect that this isn't what you want.

The barrier() didn't tell the compiler that fp->tx_bd_prod and fp->tx_bd_cons
could change.  What it did was to say that the accesses to those two variables
must be performed after all the other accesses issued by that CPU prior to the
barrier - at least as far as the compiler is concerned.

You don't need to separate the reads of tx_bd_prod and tx_bd_cons above with a
memory barrier.  They aren't ever altered in the same place.


What you want is something more like the following pseudocode.

To insert into a circular buffer:

	bd_prod = fp->tx_bd_prod;
	bd_cons = fp->tx_bd_cons;

	if (CIRC_SPACE(bd_cons, bd_prod, NUM_TX_BD) <= 0)
		goto no_space;

	/* get a tx_buf and first BD */
	tx_start_bd = &fp->tx_desc_ring[bd_prod].start_bd;
	tx_start_bd->bd_flags.as_bitfield = ETH_TX_BD_FLAGS_START_BD;
	tx_start_bd->general_data = (UNICAST_ADDRESS <<
				     ETH_TX_START_BD_ETH_ADDR_TYPE_SHIFT);
	tx_start_bd->general_data |= (1 << ETH_TX_START_BD_HDR_NBDS_SHIFT);

	smp_wmb(); /* commit buffer contents before incrementing index */

	fp->tx_bd_prod = TX_BD(bd_prod + 1);

To read from a circular buffer:

	bd_prod = fp->tx_bd_prod;
	bd_cons = fp->tx_bd_cons;

	smp_read_barrier_depends(); /* read index before reading contents */

	if (CIRC_CNT(bd_cons, bd_prod, NUM_TX_BD) <= 0)
		goto no_data;

	tx_start_bd = &fp->tx_desc_ring[bd_cons].start_bd;
	munge_descriptor(tx_start_bd);

	smp_mb(); /* finish reading descriptor before incrementing index */

	fp->tx_bd_cons = TX_BD(bd_cons + 1);

At least, I'm fairly certain that's correct.

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