Linux Virtualization - Aug 2018 - [PATCH net-next 2/2] virtio-net: get rid of unnecessary container of rq stats

On 2018/07/31 18:43, Jason Wang wrote:
> We don't maintain tx counters in rx stats any more. There's no need
> for an extra container of rq stats.
> 
> Cc: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp>
> Signed-off-by: Jason Wang <jasowang at redhat.com>
> ---
>  drivers/net/virtio_net.c | 80
++++++++++++++++++++++--------------------------
>  1 file changed, 36 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 72d3f68..14f661c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>  	u64 kicks;
>  };
>  
> -struct virtnet_rq_stat_items {
> +struct virtnet_rq_stats {
> +	struct u64_stats_sync syncp;
>  	u64 packets;
>  	u64 bytes;
>  	u64 drops;
> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>  	u64 kicks;
>  };
>  
> -struct virtnet_rq_stats {
> -	struct u64_stats_sync syncp;
> -	struct virtnet_rq_stat_items items;
> -};
I'm not thinking removing sq stat is needed but even if it is I want to
keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp on
stack in virtnet_receive. I would just remove virtnet_rx_stats if necessary.
> -
> -struct virtnet_rx_stats {
> -	struct virtnet_rq_stat_items rx;
> -};
> -
-- 
Toshiaki Makita

On 2018?07?31? 18:02, Toshiaki Makita wrote:
> On 2018/07/31 18:43, Jason Wang wrote:
>> We don't maintain tx counters in rx stats any more. There's no
need
>> for an extra container of rq stats.
>>
>> Cc: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp>
>> Signed-off-by: Jason Wang <jasowang at redhat.com>
>> ---
>>   drivers/net/virtio_net.c | 80
++++++++++++++++++++++--------------------------
>>   1 file changed, 36 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 72d3f68..14f661c 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>>   	u64 kicks;
>>   };
>>   
>> -struct virtnet_rq_stat_items {
>> +struct virtnet_rq_stats {
>> +	struct u64_stats_sync syncp;
>>   	u64 packets;
>>   	u64 bytes;
>>   	u64 drops;
>> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>>   	u64 kicks;
>>   };
>>   
>> -struct virtnet_rq_stats {
>> -	struct u64_stats_sync syncp;
>> -	struct virtnet_rq_stat_items items;
>> -};
> I'm not thinking removing sq stat is needed but even if it is I want to
> keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp on
> stack in virtnet_receive. I would just remove virtnet_rx_stats if
necessary.
It's a nop on 64bit machines. And an unsigned on 32bit. So it's overhead
could be ignored I think.

Thanks
>> -
>> -struct virtnet_rx_stats {
>> -	struct virtnet_rq_stat_items rx;
>> -};
>> -

On 2018/08/01 10:39, Jason Wang wrote:
> On 2018?07?31? 18:02, Toshiaki Makita wrote:
>> On 2018/07/31 18:43, Jason Wang wrote:
>>> We don't maintain tx counters in rx stats any more. There's
no need
>>> for an extra container of rq stats.
>>>
>>> Cc: Toshiaki Makita <makita.toshiaki at lab.ntt.co.jp>
>>> Signed-off-by: Jason Wang <jasowang at redhat.com>
>>> ---
>>> ? drivers/net/virtio_net.c | 80
>>> ++++++++++++++++++++++--------------------------
>>> ? 1 file changed, 36 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>> index 72d3f68..14f661c 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -87,7 +87,8 @@ struct virtnet_sq_stats {
>>> ????? u64 kicks;
>>> ? };
>>> ? -struct virtnet_rq_stat_items {
>>> +struct virtnet_rq_stats {
>>> +??? struct u64_stats_sync syncp;
>>> ????? u64 packets;
>>> ????? u64 bytes;
>>> ????? u64 drops;
>>> @@ -98,17 +99,8 @@ struct virtnet_rq_stat_items {
>>> ????? u64 kicks;
>>> ? };
>>> ? -struct virtnet_rq_stats {
>>> -??? struct u64_stats_sync syncp;
>>> -??? struct virtnet_rq_stat_items items;
>>> -};
>> I'm not thinking removing sq stat is needed but even if it is I
want to
>> keep virtnet_rq_stats to avoid allocating unnecessary u64_stats_syncp
on
>> stack in virtnet_receive. I would just remove virtnet_rx_stats if
>> necessary.
> 
> It's a nop on 64bit machines. And an unsigned on 32bit. So it's
overhead
> could be ignored I think.
It's not a big problem so that's OK. I just felt like you reverted
unnecessarily too much. Anyway it is already applied and I'm not
thinking of changing this any more.

-- 
Toshiaki Makita