> From: Long Li [mailto:lon...@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 <m...@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.
