netdev - Re: [PATCH 5/5] net: mvneta: replace Tx timer with a real interrupt

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: <20140114073020.GD27536@1wt.eu>
Date:	Tue, 14 Jan 2014 08:30:20 +0100
From:	Willy Tarreau <w@....eu>
To:	Arnaud Ebalard <arno@...isbad.org>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Gregory CLEMENT <gregory.clement@...e-electrons.com>,
	Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH 5/5] net: mvneta: replace Tx timer with a real interrupt

On Tue, Jan 14, 2014 at 12:22:03AM +0100, Arnaud Ebalard wrote:
> Hi Willy,
> 
> Willy Tarreau <w@....eu> writes:
> 
> > @@ -1935,14 +1907,22 @@ static int mvneta_poll(struct napi_struct *napi, int budget)
> >  
> >  	/* Read cause register */
> >  	cause_rx_tx = mvreg_read(pp, MVNETA_INTR_NEW_CAUSE) &
> > -		MVNETA_RX_INTR_MASK(rxq_number);
> > +		(MVNETA_RX_INTR_MASK(rxq_number) | MVNETA_TX_INTR_MASK(txq_number));
> > +
> > +	/* Release Tx descriptors */
> > +	if (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL) {
> > +		int tx_todo = 0;
> > +
> > +		mvneta_tx_done_gbe(pp, (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL), &tx_todo);
> > +		cause_rx_tx &= ~MVNETA_TX_INTR_MASK_ALL;
> > +	}
> 
> Unless I missed something, tx_todo above is just here to make the
> compiler happy w/ current prototype of mvneta_tx_done_gbe() but is
> otherwise unused: you could simply remove the third parameter of the
> function (it is only used here) and remove tx_todo.

A number of such changes could be done but should be merged separately,
along with the cleanup and improvement series.

> Additionally, as you do not use the return value of the function, you
> could probably make it void and spare some additional cycles by removing
> the computation of the return value. While at it, mvneta_txq_done()
> could also be made void.
> 
> The patch below gives the idea, it's compile-tested only and applies on
> your whole set (fixes + perf).

You should propose your patches for net-next on top of my series, really,
it's not too late.

Please see my comments below.

> Index: linux/drivers/net/ethernet/marvell/mvneta.c
> ===================================================================
> --- linux.orig/drivers/net/ethernet/marvell/mvneta.c	2014-01-14 00:07:18.728729578 +0100
> +++ linux/drivers/net/ethernet/marvell/mvneta.c	2014-01-14 00:11:57.740949448 +0100
> @@ -1314,25 +1314,23 @@
>  }
>  
>  /* Handle end of transmission */
> -static int mvneta_txq_done(struct mvneta_port *pp,
> +static void mvneta_txq_done(struct mvneta_port *pp,
>  			   struct mvneta_tx_queue *txq)
>  {
>  	struct netdev_queue *nq = netdev_get_tx_queue(pp->dev, txq->id);
>  	int tx_done;
>  
>  	tx_done = mvneta_txq_sent_desc_proc(pp, txq);
> -	if (tx_done == 0)
> -		return tx_done;
> -	mvneta_txq_bufs_free(pp, txq, tx_done);
> +	if (tx_done) {
> +		mvneta_txq_bufs_free(pp, txq, tx_done);

Better just use "if (tx_done == 0) return" above and avoid adding an
extra indent level by inverting the if, that makes the code more readable.

> -	txq->count -= tx_done;
> +		txq->count -= tx_done;
>  
> -	if (netif_tx_queue_stopped(nq)) {
> -		if (txq->size - txq->count >= MAX_SKB_FRAGS + 1)
> -			netif_tx_wake_queue(nq);
> +		if (netif_tx_queue_stopped(nq)) {
> +			if (txq->size - txq->count >= MAX_SKB_FRAGS + 1)
> +				netif_tx_wake_queue(nq);
> +		}
>  	}
> -
> -	return tx_done;
>  }
>  
>  static void *mvneta_frag_alloc(const struct mvneta_port *pp)
> @@ -1704,30 +1702,23 @@
>  /* Handle tx done - called in softirq context. The <cause_tx_done> argument
>   * must be a valid cause according to MVNETA_TXQ_INTR_MASK_ALL.
>   */
> -static u32 mvneta_tx_done_gbe(struct mvneta_port *pp, u32 cause_tx_done,
> -			      int *tx_todo)
> +static void mvneta_tx_done_gbe(struct mvneta_port *pp, u32 cause_tx_done)
>  {
>  	struct mvneta_tx_queue *txq;
> -	u32 tx_done = 0;
>  	struct netdev_queue *nq;
>  
> -	*tx_todo = 0;
>  	while (cause_tx_done) {
>  		txq = mvneta_tx_done_policy(pp, cause_tx_done);
>  
>  		nq = netdev_get_tx_queue(pp->dev, txq->id);
>  		__netif_tx_lock(nq, smp_processor_id());
>  
> -		if (txq->count) {
> -			tx_done += mvneta_txq_done(pp, txq);
> -			*tx_todo += txq->count;
> -		}
> +		if (txq->count)
> +			mvneta_txq_done(pp, txq);
>  
>  		__netif_tx_unlock(nq);
>  		cause_tx_done &= ~((1 << txq->id));
>  	}
> -
> -	return tx_done;
>  }

Seems fine.

>  /* Compute crc8 of the specified address, using a unique algorithm ,
> @@ -1961,9 +1952,7 @@
>  
>  	/* Release Tx descriptors */
>  	if (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL) {
> -		int tx_todo = 0;
> -
> -		mvneta_tx_done_gbe(pp, (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL), &tx_todo);
> +		mvneta_tx_done_gbe(pp, (cause_rx_tx & MVNETA_TX_INTR_MASK_ALL));
>  		cause_rx_tx &= ~MVNETA_TX_INTR_MASK_ALL;
>  	}

Seems fine as well.

Thanks!
Willy

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