netdev - Re: [PATCH net-next v3 1/6] tsnep: Replace modulo operation with mask

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: <ZEFKzuPKGRv0bO35@boxer>
Date:   Thu, 20 Apr 2023 16:23:10 +0200
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Gerhard Engleder <gerhard@...leder-embedded.com>
CC:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <davem@...emloft.net>, <kuba@...nel.org>, <edumazet@...gle.com>,
        <pabeni@...hat.com>, <bjorn@...nel.org>,
        <magnus.karlsson@...el.com>, <jonathan.lemon@...il.com>
Subject: Re: [PATCH net-next v3 1/6] tsnep: Replace modulo operation with mask

On Tue, Apr 18, 2023 at 09:04:54PM +0200, Gerhard Engleder wrote:
> TX/RX ring size is static and power of 2 to enable compiler to optimize
> modulo operation to mask operation. Make this optimization already in
> the code and don't rely on the compiler.

I think this came out of my review, so:
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>

Does this give you a minor perf boost?

> 
> Signed-off-by: Gerhard Engleder <gerhard@...leder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/tsnep.h      |  1 +
>  drivers/net/ethernet/engleder/tsnep_main.c | 28 +++++++++++-----------
>  2 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/engleder/tsnep.h b/drivers/net/ethernet/engleder/tsnep.h
> index 058c2bcf31a7..1de26aec78d3 100644
> --- a/drivers/net/ethernet/engleder/tsnep.h
> +++ b/drivers/net/ethernet/engleder/tsnep.h
> @@ -18,6 +18,7 @@
>  #define TSNEP "tsnep"
>  
>  #define TSNEP_RING_SIZE 256
> +#define TSNEP_RING_MASK (TSNEP_RING_SIZE - 1)
>  #define TSNEP_RING_RX_REFILL 16
>  #define TSNEP_RING_RX_REUSE (TSNEP_RING_SIZE - TSNEP_RING_SIZE / 4)
>  #define TSNEP_RING_ENTRIES_PER_PAGE (PAGE_SIZE / TSNEP_DESC_SIZE)
> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index ed1b6102cfeb..3d15e673894a 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -292,7 +292,7 @@ static int tsnep_tx_ring_init(struct tsnep_tx *tx)
>  	}
>  	for (i = 0; i < TSNEP_RING_SIZE; i++) {
>  		entry = &tx->entry[i];
> -		next_entry = &tx->entry[(i + 1) % TSNEP_RING_SIZE];
> +		next_entry = &tx->entry[(i + 1) & TSNEP_RING_MASK];
>  		entry->desc->next = __cpu_to_le64(next_entry->desc_dma);
>  	}
>  
> @@ -381,7 +381,7 @@ static int tsnep_tx_map(struct sk_buff *skb, struct tsnep_tx *tx, int count)
>  	int i;
>  
>  	for (i = 0; i < count; i++) {
> -		entry = &tx->entry[(tx->write + i) % TSNEP_RING_SIZE];
> +		entry = &tx->entry[(tx->write + i) & TSNEP_RING_MASK];
>  
>  		if (!i) {
>  			len = skb_headlen(skb);
> @@ -419,7 +419,7 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
>  	int i;
>  
>  	for (i = 0; i < count; i++) {
> -		entry = &tx->entry[(index + i) % TSNEP_RING_SIZE];
> +		entry = &tx->entry[(index + i) & TSNEP_RING_MASK];
>  
>  		if (entry->len) {
>  			if (entry->type & TSNEP_TX_TYPE_SKB)
> @@ -481,9 +481,9 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>  		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>  
>  	for (i = 0; i < count; i++)
> -		tsnep_tx_activate(tx, (tx->write + i) % TSNEP_RING_SIZE, length,
> +		tsnep_tx_activate(tx, (tx->write + i) & TSNEP_RING_MASK, length,
>  				  i == count - 1);
> -	tx->write = (tx->write + count) % TSNEP_RING_SIZE;
> +	tx->write = (tx->write + count) & TSNEP_RING_MASK;
>  
>  	skb_tx_timestamp(skb);
>  
> @@ -516,7 +516,7 @@ static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
>  	frag = NULL;
>  	len = xdpf->len;
>  	for (i = 0; i < count; i++) {
> -		entry = &tx->entry[(tx->write + i) % TSNEP_RING_SIZE];
> +		entry = &tx->entry[(tx->write + i) & TSNEP_RING_MASK];
>  		if (type & TSNEP_TX_TYPE_XDP_NDO) {
>  			data = unlikely(frag) ? skb_frag_address(frag) :
>  						xdpf->data;
> @@ -589,9 +589,9 @@ static bool tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
>  	length = retval;
>  
>  	for (i = 0; i < count; i++)
> -		tsnep_tx_activate(tx, (tx->write + i) % TSNEP_RING_SIZE, length,
> +		tsnep_tx_activate(tx, (tx->write + i) & TSNEP_RING_MASK, length,
>  				  i == count - 1);
> -	tx->write = (tx->write + count) % TSNEP_RING_SIZE;
> +	tx->write = (tx->write + count) & TSNEP_RING_MASK;
>  
>  	/* descriptor properties shall be valid before hardware is notified */
>  	dma_wmb();
> @@ -691,7 +691,7 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  		/* xdpf is union with skb */
>  		entry->skb = NULL;
>  
> -		tx->read = (tx->read + count) % TSNEP_RING_SIZE;
> +		tx->read = (tx->read + count) & TSNEP_RING_MASK;
>  
>  		tx->packets++;
>  		tx->bytes += length + ETH_FCS_LEN;
> @@ -839,7 +839,7 @@ static int tsnep_rx_ring_init(struct tsnep_rx *rx)
>  
>  	for (i = 0; i < TSNEP_RING_SIZE; i++) {
>  		entry = &rx->entry[i];
> -		next_entry = &rx->entry[(i + 1) % TSNEP_RING_SIZE];
> +		next_entry = &rx->entry[(i + 1) & TSNEP_RING_MASK];
>  		entry->desc->next = __cpu_to_le64(next_entry->desc_dma);
>  	}
>  
> @@ -925,7 +925,7 @@ static int tsnep_rx_refill(struct tsnep_rx *rx, int count, bool reuse)
>  	int retval;
>  
>  	for (i = 0; i < count && !alloc_failed; i++) {
> -		index = (rx->write + i) % TSNEP_RING_SIZE;
> +		index = (rx->write + i) & TSNEP_RING_MASK;
>  
>  		retval = tsnep_rx_alloc_buffer(rx, index);
>  		if (unlikely(retval)) {
> @@ -945,7 +945,7 @@ static int tsnep_rx_refill(struct tsnep_rx *rx, int count, bool reuse)
>  	}
>  
>  	if (enable) {
> -		rx->write = (rx->write + i) % TSNEP_RING_SIZE;
> +		rx->write = (rx->write + i) & TSNEP_RING_MASK;
>  
>  		/* descriptor properties shall be valid before hardware is
>  		 * notified
> @@ -1090,7 +1090,7 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
>  				 * empty RX ring, thus buffer cannot be used for
>  				 * RX processing
>  				 */
> -				rx->read = (rx->read + 1) % TSNEP_RING_SIZE;
> +				rx->read = (rx->read + 1) & TSNEP_RING_MASK;
>  				desc_available++;
>  
>  				rx->dropped++;
> @@ -1117,7 +1117,7 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
>  		 */
>  		length -= TSNEP_RX_INLINE_METADATA_SIZE;
>  
> -		rx->read = (rx->read + 1) % TSNEP_RING_SIZE;
> +		rx->read = (rx->read + 1) & TSNEP_RING_MASK;
>  		desc_available++;
>  
>  		if (prog) {
> -- 
> 2.30.2
>