[PATCH] netvsc: optimize stats counters performance

[PATCH] netvsc: optimize stats counters performance

[Morten Brørup]

Optimized the performance of updating the statistics counters by reducing
the number of branches.

Ordered the packet size comparisons according to the probability with
typical internet traffic mix.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 drivers/net/netvsc/hn_rxtx.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index 9bf1ec5509..b704b2c971 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -110,30 +110,18 @@ hn_update_packet_stats(struct hn_stats *stats, const struct rte_mbuf *m)
 	uint32_t s = m->pkt_len;
 	const struct rte_ether_addr *ea;
 
-	if (s == 64) {
-		stats->size_bins[1]++;
-	} else if (s > 64 && s < 1024) {
-		uint32_t bin;
-
-		/* count zeros, and offset into correct bin */
-		bin = (sizeof(s) * 8) - rte_clz32(s) - 5;
-		stats->size_bins[bin]++;
-	} else {
-		if (s < 64)
-			stats->size_bins[0]++;
-		else if (s < 1519)
-			stats->size_bins[6]++;
-		else
-			stats->size_bins[7]++;
-	}
+	if (s >= 1024)
+		stats->size_bins[6 + (s > 1518)]++;
+	else if (s <= 64)
+		stats->size_bins[s >> 6]++;
+	else
+		stats->size_bins[32UL - rte_clz32(s) - 5]++;
 
 	ea = rte_pktmbuf_mtod(m, const struct rte_ether_addr *);
-	if (rte_is_multicast_ether_addr(ea)) {
-		if (rte_is_broadcast_ether_addr(ea))
-			stats->broadcast++;
-		else
-			stats->multicast++;
-	}
+	RTE_BUILD_BUG_ON(offsetof(struct hn_stats, broadcast) !=
+			offsetof(struct hn_stats, multicast) + sizeof(uint64_t));
+	if (unlikely(rte_is_multicast_ether_addr(ea)))
+		(&stats->multicast)[rte_is_broadcast_ether_addr(ea)]++;
 }
 
 static inline unsigned int hn_rndis_pktlen(const struct rndis_packet_msg *pkt)
-- 
2.43.0

RE: [PATCH] netvsc: optimize stats counters performance

[Long Li]

> Subject: [PATCH] netvsc: optimize stats counters performance
> 
> Optimized the performance of updating the statistics counters by reducing the
> number of branches.
> 
> Ordered the packet size comparisons according to the probability with typical
> internet traffic mix.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  drivers/net/netvsc/hn_rxtx.c | 32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c index
> 9bf1ec5509..b704b2c971 100644
> --- a/drivers/net/netvsc/hn_rxtx.c
> +++ b/drivers/net/netvsc/hn_rxtx.c
> @@ -110,30 +110,18 @@ hn_update_packet_stats(struct hn_stats *stats,
> const struct rte_mbuf *m)
>  	uint32_t s = m->pkt_len;
>  	const struct rte_ether_addr *ea;
> 
> -	if (s == 64) {
> -		stats->size_bins[1]++;
> -	} else if (s > 64 && s < 1024) {
> -		uint32_t bin;
> -
> -		/* count zeros, and offset into correct bin */
> -		bin = (sizeof(s) * 8) - rte_clz32(s) - 5;
> -		stats->size_bins[bin]++;
> -	} else {
> -		if (s < 64)
> -			stats->size_bins[0]++;
> -		else if (s < 1519)
> -			stats->size_bins[6]++;
> -		else
> -			stats->size_bins[7]++;
> -	}
> +	if (s >= 1024)
> +		stats->size_bins[6 + (s > 1518)]++;
> +	else if (s <= 64)
> +		stats->size_bins[s >> 6]++;
> +	else
> +		stats->size_bins[32UL - rte_clz32(s) - 5]++;

This part looks good.

> 
>  	ea = rte_pktmbuf_mtod(m, const struct rte_ether_addr *);
> -	if (rte_is_multicast_ether_addr(ea)) {
> -		if (rte_is_broadcast_ether_addr(ea))
> -			stats->broadcast++;
> -		else
> -			stats->multicast++;
> -	}
> +	RTE_BUILD_BUG_ON(offsetof(struct hn_stats, broadcast) !=
> +			offsetof(struct hn_stats, multicast) + sizeof(uint64_t));
> +	if (unlikely(rte_is_multicast_ether_addr(ea)))
> +		(&stats->multicast)[rte_is_broadcast_ether_addr(ea)]++;
>  }

This makes the code a little harder to read. How about just add "unlikely" to rte_is_multicast_ether_addr(ea) and keep the rest unchanged?

Thank you,

Long

RE: [PATCH] netvsc: optimize stats counters performance

[Morten Brørup]

> From: Long Li [mailto:longli@microsoft.com]
> Sent: Friday, 2 August 2024 18.49
> 
> > Subject: [PATCH] netvsc: optimize stats counters performance
> >
> > Optimized the performance of updating the statistics counters by
> reducing the
> > number of branches.
> >
> > Ordered the packet size comparisons according to the probability with
> typical
> > internet traffic mix.
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  drivers/net/netvsc/hn_rxtx.c | 32 ++++++++++----------------------
> >  1 file changed, 10 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/net/netvsc/hn_rxtx.c
> b/drivers/net/netvsc/hn_rxtx.c index
> > 9bf1ec5509..b704b2c971 100644
> > --- a/drivers/net/netvsc/hn_rxtx.c
> > +++ b/drivers/net/netvsc/hn_rxtx.c
> > @@ -110,30 +110,18 @@ hn_update_packet_stats(struct hn_stats *stats,
> > const struct rte_mbuf *m)
> >  	uint32_t s = m->pkt_len;
> >  	const struct rte_ether_addr *ea;
> >
> > -	if (s == 64) {
> > -		stats->size_bins[1]++;
> > -	} else if (s > 64 && s < 1024) {
> > -		uint32_t bin;
> > -
> > -		/* count zeros, and offset into correct bin */
> > -		bin = (sizeof(s) * 8) - rte_clz32(s) - 5;
> > -		stats->size_bins[bin]++;
> > -	} else {
> > -		if (s < 64)
> > -			stats->size_bins[0]++;
> > -		else if (s < 1519)
> > -			stats->size_bins[6]++;
> > -		else
> > -			stats->size_bins[7]++;
> > -	}
> > +	if (s >= 1024)
> > +		stats->size_bins[6 + (s > 1518)]++;
> > +	else if (s <= 64)
> > +		stats->size_bins[s >> 6]++;
> > +	else
> > +		stats->size_bins[32UL - rte_clz32(s) - 5]++;
> 
> This part looks good.
> 
> >
> >  	ea = rte_pktmbuf_mtod(m, const struct rte_ether_addr *);
> > -	if (rte_is_multicast_ether_addr(ea)) {
> > -		if (rte_is_broadcast_ether_addr(ea))
> > -			stats->broadcast++;
> > -		else
> > -			stats->multicast++;
> > -	}
> > +	RTE_BUILD_BUG_ON(offsetof(struct hn_stats, broadcast) !=
> > +			offsetof(struct hn_stats, multicast) +
> sizeof(uint64_t));
> > +	if (unlikely(rte_is_multicast_ether_addr(ea)))
> > +		(&stats->multicast)[rte_is_broadcast_ether_addr(ea)]++;
> >  }
> 
> This makes the code a little harder to read.

I agree it is somewhat convoluted.
It's a tradeoff... I preferred performance at the cost of making the code somewhat harder to read.
The RTE_BUILD_BUG_ON() also helps showing what is going on with the weird indexing.

> How about just add
> "unlikely" to rte_is_multicast_ether_addr(ea) and keep the rest
> unchanged?

Then I could also add "unlikely" to is_broadcast().
In theory, there should be minimal broadcast traffic in any normal network. (Except when experiencing broadcast storms due to network loops or other network problems.)
But in reality, I think most real life networks see less multicast (non-broadcast) than broadcast traffic.

I don' think the following alternative makes the code significantly more readable than the method in this patch, but I'll mention it for the sake of discussion:

I could modify the hn_stats type like this:

struct hn_stats {
other fields...
+	union {
+		struct {
			uint64_t	multicast;
			uint64_t	broadcast;
+		};
+		uint64_t	multicast_broadcast[2];
+	};
other fields...
};

So the code would become:

+	if (unlikely(rte_is_multicast_ether_addr(ea)))
+		 stats->multicast_broadcast[rte_is_broadcast_ether_addr(ea)]++;

Whatever we decide, we should use the same method in all three patches (netvsc, virtio, vhost-user).
The choices are:
1. Stick with the original code (with two branches), and add unlikely().
2. Use the method provided in this patch (with only one branch).
3. Introduce a multicast_broadcast[2] to the stats structure as described above (also with only one branch).

@Long, @Wei, @Maxime, @Chenbo, @Stephen and anyone else interested, please cast your votes.
Comments are also welcome! :-)

I'm in favor of #2.

Re: [PATCH] netvsc: optimize stats counters performance

[Stephen Hemminger]

On Fri, 2 Aug 2024 19:28:26 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > >  	ea = rte_pktmbuf_mtod(m, const struct rte_ether_addr *);
> > > -	if (rte_is_multicast_ether_addr(ea)) {
> > > -		if (rte_is_broadcast_ether_addr(ea))
> > > -			stats->broadcast++;
> > > -		else
> > > -			stats->multicast++;
> > > -	}
> > > +	RTE_BUILD_BUG_ON(offsetof(struct hn_stats, broadcast) !=
> > > +			offsetof(struct hn_stats, multicast) +  
> > sizeof(uint64_t));  
> > > +	if (unlikely(rte_is_multicast_ether_addr(ea)))
> > > +		(&stats->multicast)[rte_is_broadcast_ether_addr(ea)]++;
> > >  }  
> > 
> > This makes the code a little harder to read.  
> 
> I agree it is somewhat convoluted.
> It's a tradeoff... I preferred performance at the cost of making the code somewhat harder to read.
> The RTE_BUILD_BUG_ON() also helps showing what is going on with the weird indexing.

Optimizing for multicast packets is not worth bothering.
Keep the original code it is simpler.