netdev - Re: [WIP] net+mlx4: auto doorbell

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: <1480521386.18162.189.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Wed, 30 Nov 2016 07:56:26 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Rick Jones <rick.jones2@....com>, netdev@...r.kernel.org,
        Saeed Mahameed <saeedm@...lanox.com>,
        Tariq Toukan <tariqt@...lanox.com>,
        Achiad Shochat <achiad@...lanox.com>
Subject: Re: [WIP] net+mlx4: auto doorbell

On Wed, 2016-11-30 at 12:38 +0100, Jesper Dangaard Brouer wrote:
> I've played with a somewhat similar patch (from Achiad Shochat) for
> mlx5 (attached).  While it gives huge improvements, the problem I ran
> into was that; TX performance became a function of the TX completion
> time/interrupt and could easily be throttled if configured too
> high/slow.
> 
> Can your patch be affected by this too?

Like all TX business, you should know this Jesper.

No need to constantly remind us something very well known.

> 
> Adjustable via:
>  ethtool -C mlx5p2 tx-usecs 16 tx-frames 32
>  

Generally speaking these settings impact TX, throughput/latencies.

Since NIC IRQ then trigger softirqs, we already know that IRQ handling
is critical, and some tuning can help, depending on particular load.

Now the trick is when you want a generic kernel, being deployed on hosts
shared by millions of jobs.


> 
> On Mon, 28 Nov 2016 22:58:36 -0800 Eric Dumazet <eric.dumazet@...il.com> wrote:
> 
> > I have a WIP, that increases pktgen rate by 75 % on mlx4 when bulking is
> > not used.
> > 
> > lpaa23:~# echo 0 >/sys/class/net/eth0/doorbell_opt 
> > lpaa23:~# sar -n DEV 1 10|grep eth0
> [...]
> > Average:         eth0      9.50 5707925.60      0.99 585285.69      0.00      0.00      0.50
> > lpaa23:~# echo 1 >/sys/class/net/eth0/doorbell_opt 
> > lpaa23:~# sar -n DEV 1 10|grep eth0
> [...]
> > Average:         eth0      2.40 9985214.90      0.31 1023874.60      0.00      0.00      0.50
> 
> These +75% number is pktgen without "burst", and definitely show that
> your patch activate xmit_more.
> What is the pps performance number when using pktgen "burst" option?

About the same really. About all packets now get the xmit_more effect.

> 
> 
> > And about 11 % improvement on an mono-flow UDP_STREAM test.
> > 
> > skb_set_owner_w() is now the most consuming function.
> > 
> > 
> > lpaa23:~# ./udpsnd -4 -H 10.246.7.152 -d 2 &
> > [1] 13696
> > lpaa23:~# echo 0 >/sys/class/net/eth0/doorbell_opt
> > lpaa23:~# sar -n DEV 1 10|grep eth0
> [...]
> > Average:         eth0      9.00 1307380.50      1.00 308356.18      0.00      0.00      0.50
> > lpaa23:~# echo 3 >/sys/class/net/eth0/doorbell_opt
> > lpaa23:~# sar -n DEV 1 10|grep eth0
> [...]
> > Average:         eth0      3.10 1459558.20      0.44 344267.57      0.00      0.00      0.50
> 
> The +11% number seems consistent with my perf observations that approx
> 12% was "fakely" spend on the xmit spin_lock.
> 
> 
> [...]
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> > index 4b597dca5c52..affebb435679 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> [...]
> > -static inline bool mlx4_en_is_tx_ring_full(struct mlx4_en_tx_ring *ring)
> > +static inline bool mlx4_en_is_tx_ring_full(const struct mlx4_en_tx_ring *ring)
> >  {
> > -	return ring->prod - ring->cons > ring->full_size;
> > +	return READ_ONCE(ring->prod) - READ_ONCE(ring->cons) > ring->full_size;
> >  }
> [...]
> 
> > @@ -1033,6 +1058,14 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
> >  	}
> >  	send_doorbell = !skb->xmit_more || netif_xmit_stopped(ring->tx_queue);
> >  
> > +	/* Doorbell avoidance : We can omit doorbell if we know a TX completion
> > +	 * will happen shortly.
> > +	 */
> > +	if (send_doorbell &&
> > +	    dev->doorbell_opt &&
> > +	    (s32)(READ_ONCE(ring->prod_bell) - READ_ONCE(ring->ncons)) > 0)
> 
> It would be nice with a function call with an appropriate name, instead
> of an open-coded queue size check.  I'm also confused by the "ncons" name.
> 
> > +		send_doorbell = false;
> > +
> [...]
> 
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> > index 574bcbb1b38f..c3fd0deda198 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> > +++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
> > @@ -280,6 +280,7 @@ struct mlx4_en_tx_ring {
> >  	 */
> >  	u32			last_nr_txbb;
> >  	u32			cons;
> > +	u32			ncons;
> 
> Maybe we can find a better name than "ncons" ?

Thats because 'cons' in this driver really means 'old cons' 

and new cons = old cons + last_nr_txbb;


> 
> >  	unsigned long		wake_queue;
> >  	struct netdev_queue	*tx_queue;
> >  	u32			(*free_tx_desc)(struct mlx4_en_priv *priv,
> > @@ -290,6 +291,7 @@ struct mlx4_en_tx_ring {
> >  
> >  	/* cache line used and dirtied in mlx4_en_xmit() */
> >  	u32			prod ____cacheline_aligned_in_smp;
> > +	u32			prod_bell;
> 
> Good descriptive variable name.
> 
> >  	unsigned int		tx_dropped;
> >  	unsigned long		bytes;
> >  	unsigned long		packets;
> 
>