Re: [PATCH v5] virtio: optimize stats counters performance

["lihuisong (C)" <lihuisong@huawei.com>]

在 2024/8/2 19:27, Morten Brørup 写道:
>> From: lihuisong (C) [mailto:lihuisong@huawei.com]
>> Sent: Friday, 2 August 2024 04.23
>>
>> 在 2024/8/2 0:03, Morten Brørup 写道:
>>>    void
>>> -virtio_update_packet_stats(struct virtnet_stats *stats, struct
>> rte_mbuf *mbuf)
>>> +virtio_update_packet_stats(struct virtnet_stats *const stats,
>>> +		const struct rte_mbuf *const mbuf)
>> The two const is also for performace?  Is there gain?
> The "const struct rte_mbuf * mbuf" informs the optimizer that the function does not modify the mbuf.  This is for the benefit of callers of the function; they can rely on the mbuf being unchanged when the function returns.
> So, if the optimizer has cached some mbuf field before calling the function, it does not need to read the mbuf field again, but can continue using the cached value after the function call.
>
> The two "type *const ptr" probably make no performance difference with the compilers and function call conventions used by the CPU architectures supported by DPDK.
ok
>
>>> +	RTE_BUILD_BUG_ON(offsetof(struct virtnet_stats, broadcast) !=
>>> +			offsetof(struct virtnet_stats, multicast) +
>> sizeof(uint64_t));
>>> +	if (unlikely(rte_is_multicast_ether_addr(ea)))
>>> +		(&stats->multicast)[rte_is_broadcast_ether_addr(ea)]++;
>> The "rte_is_broadcast_ether_addr(ea) " will be calculated twice if
>> packet is mulitcast.
>> How about coding like:
>> -->
>> is_mulitcast = rte_is_multicast_ether_addr(ea);
>> if (unlikely(is_mulitcast))
>> (&stats->multicast)[rte_is_broadcast_ether_addr(ea)]++;
> I don't think "rte_is_broadcast_ether_addr(ea)" is calculated twice for multicast packets.
> My code essentially does this:
> 	if (mc(ea))
> 		stats[bc(ea)]++;
>
> Changing to this shouldn't make a difference:
> 	m = mc(ea);
> 	if (m)
> 		stats[bc(ea)]++;
Yeah，you are right.
>