[PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Li RongQing]

This place is fetching the stats, so u64_stats_fetch_begin
and u64_stats_fetch_retry should be used

Fixes: 6208799553a8 ("virtio-net: support rx netdim")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/net/virtio_net.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 61a57d1..b669e73 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2332,16 +2332,18 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
 static void virtnet_rx_dim_update(struct virtnet_info *vi, struct receive_queue *rq)
 {
 	struct dim_sample cur_sample = {};
+	unsigned int start;
 
 	if (!rq->packets_in_napi)
 		return;
 
-	u64_stats_update_begin(&rq->stats.syncp);
-	dim_update_sample(rq->calls,
-			  u64_stats_read(&rq->stats.packets),
-			  u64_stats_read(&rq->stats.bytes),
-			  &cur_sample);
-	u64_stats_update_end(&rq->stats.syncp);
+	do {
+		start = u64_stats_fetch_begin(&rq->stats.syncp);
+		dim_update_sample(rq->calls,
+				u64_stats_read(&rq->stats.packets),
+				u64_stats_read(&rq->stats.bytes),
+				&cur_sample);
+	} while (u64_stats_fetch_retry(&rq->stats.syncp, start));
 
 	net_dim(&rq->dim, cur_sample);
 	rq->packets_in_napi = 0;
-- 
2.9.4

Re: [PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Heng Qi]

On Wed, 19 Jun 2024 10:55:29 +0800, Li RongQing <lirongqing@baidu.com> wrote:
> This place is fetching the stats, so u64_stats_fetch_begin
> and u64_stats_fetch_retry should be used

Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>

Thanks!

> 
> Fixes: 6208799553a8 ("virtio-net: support rx netdim")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  drivers/net/virtio_net.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 61a57d1..b669e73 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2332,16 +2332,18 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
>  static void virtnet_rx_dim_update(struct virtnet_info *vi, struct receive_queue *rq)
>  {
>  	struct dim_sample cur_sample = {};
> +	unsigned int start;
>  
>  	if (!rq->packets_in_napi)
>  		return;
>  
> -	u64_stats_update_begin(&rq->stats.syncp);
> -	dim_update_sample(rq->calls,
> -			  u64_stats_read(&rq->stats.packets),
> -			  u64_stats_read(&rq->stats.bytes),
> -			  &cur_sample);
> -	u64_stats_update_end(&rq->stats.syncp);
> +	do {
> +		start = u64_stats_fetch_begin(&rq->stats.syncp);
> +		dim_update_sample(rq->calls,
> +				u64_stats_read(&rq->stats.packets),
> +				u64_stats_read(&rq->stats.bytes),
> +				&cur_sample);
> +	} while (u64_stats_fetch_retry(&rq->stats.syncp, start));
>  
>  	net_dim(&rq->dim, cur_sample);
>  	rq->packets_in_napi = 0;
> -- 
> 2.9.4
> 

Re: [PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Jiri Pirko]

Wed, Jun 19, 2024 at 04:55:29AM CEST, lirongqing@baidu.com wrote:
>This place is fetching the stats, so u64_stats_fetch_begin
>and u64_stats_fetch_retry should be used
>
>Fixes: 6208799553a8 ("virtio-net: support rx netdim")
>Signed-off-by: Li RongQing <lirongqing@baidu.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Michael S. Tsirkin]

On Wed, Jun 19, 2024 at 10:55:29AM +0800, Li RongQing wrote:
> This place is fetching the stats, so u64_stats_fetch_begin
> and u64_stats_fetch_retry should be used
> 
> Fixes: 6208799553a8 ("virtio-net: support rx netdim")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/virtio_net.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 61a57d1..b669e73 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2332,16 +2332,18 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
>  static void virtnet_rx_dim_update(struct virtnet_info *vi, struct receive_queue *rq)
>  {
>  	struct dim_sample cur_sample = {};
> +	unsigned int start;
>  
>  	if (!rq->packets_in_napi)
>  		return;
>  
> -	u64_stats_update_begin(&rq->stats.syncp);
> -	dim_update_sample(rq->calls,
> -			  u64_stats_read(&rq->stats.packets),
> -			  u64_stats_read(&rq->stats.bytes),
> -			  &cur_sample);
> -	u64_stats_update_end(&rq->stats.syncp);
> +	do {
> +		start = u64_stats_fetch_begin(&rq->stats.syncp);
> +		dim_update_sample(rq->calls,
> +				u64_stats_read(&rq->stats.packets),
> +				u64_stats_read(&rq->stats.bytes),
> +				&cur_sample);
> +	} while (u64_stats_fetch_retry(&rq->stats.syncp, start));
>  
>  	net_dim(&rq->dim, cur_sample);
>  	rq->packets_in_napi = 0;
> -- 
> 2.9.4

Re: [PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Jason Wang]

On Wed, Jun 19, 2024 at 10:56 AM Li RongQing <lirongqing@baidu.com> wrote:
>
> This place is fetching the stats, so u64_stats_fetch_begin
> and u64_stats_fetch_retry should be used
>
> Fixes: 6208799553a8 ("virtio-net: support rx netdim")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Jakub Kicinski]

On Wed, 19 Jun 2024 10:55:29 +0800 Li RongQing wrote:
> This place is fetching the stats, so u64_stats_fetch_begin
> and u64_stats_fetch_retry should be used
> 
> Fixes: 6208799553a8 ("virtio-net: support rx netdim")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  drivers/net/virtio_net.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 61a57d1..b669e73 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2332,16 +2332,18 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
>  static void virtnet_rx_dim_update(struct virtnet_info *vi, struct receive_queue *rq)
>  {
>  	struct dim_sample cur_sample = {};
> +	unsigned int start;
>  
>  	if (!rq->packets_in_napi)
>  		return;
>  
> -	u64_stats_update_begin(&rq->stats.syncp);
> -	dim_update_sample(rq->calls,
> -			  u64_stats_read(&rq->stats.packets),
> -			  u64_stats_read(&rq->stats.bytes),
> -			  &cur_sample);
> -	u64_stats_update_end(&rq->stats.syncp);
> +	do {
> +		start = u64_stats_fetch_begin(&rq->stats.syncp);
> +		dim_update_sample(rq->calls,
> +				u64_stats_read(&rq->stats.packets),
> +				u64_stats_read(&rq->stats.bytes),
> +				&cur_sample);
> +	} while (u64_stats_fetch_retry(&rq->stats.syncp, start));

Did you by any chance use an automated tool of any sort to find this
issue or generate the fix?

I don't think this is actually necessary here, you're in the same
context as the updater of the stats, you don't need any protection.
You can remove u64_stats_update_begin() / end() (in net-next, there's
no bug).

I won't comment on implications of calling dim_update_sample() in 
a loop.

Please make sure you answer my "did you use a tool" question, I'm
really curious.
-- 
pw-bot: cr

Re: [PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 07:09:08AM -0700, Jakub Kicinski wrote:
> On Wed, 19 Jun 2024 10:55:29 +0800 Li RongQing wrote:
> > This place is fetching the stats, so u64_stats_fetch_begin
> > and u64_stats_fetch_retry should be used
> > 
> > Fixes: 6208799553a8 ("virtio-net: support rx netdim")
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >  drivers/net/virtio_net.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 61a57d1..b669e73 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2332,16 +2332,18 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
> >  static void virtnet_rx_dim_update(struct virtnet_info *vi, struct receive_queue *rq)
> >  {
> >  	struct dim_sample cur_sample = {};
> > +	unsigned int start;
> >  
> >  	if (!rq->packets_in_napi)
> >  		return;
> >  
> > -	u64_stats_update_begin(&rq->stats.syncp);
> > -	dim_update_sample(rq->calls,
> > -			  u64_stats_read(&rq->stats.packets),
> > -			  u64_stats_read(&rq->stats.bytes),
> > -			  &cur_sample);
> > -	u64_stats_update_end(&rq->stats.syncp);
> > +	do {
> > +		start = u64_stats_fetch_begin(&rq->stats.syncp);
> > +		dim_update_sample(rq->calls,
> > +				u64_stats_read(&rq->stats.packets),
> > +				u64_stats_read(&rq->stats.bytes),
> > +				&cur_sample);
> > +	} while (u64_stats_fetch_retry(&rq->stats.syncp, start));
> 
> Did you by any chance use an automated tool of any sort to find this
> issue or generate the fix?
> 
> I don't think this is actually necessary here, you're in the same
> context as the updater of the stats, you don't need any protection.
> You can remove u64_stats_update_begin() / end() (in net-next, there's
> no bug).
> 
> I won't comment on implications of calling dim_update_sample() in 
> a loop.

I didn't realize there are any - it seems to be idempotent, no?


> Please make sure you answer my "did you use a tool" question, I'm
> really curious.
> -- 
> pw-bot: cr

RE: [????] Re: [PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Li,Rongqing]

> Did you by any chance use an automated tool of any sort to find this issue or
> generate the fix?
> 
> I don't think this is actually necessary here, you're in the same context as the
> updater of the stats, you don't need any protection.
> You can remove u64_stats_update_begin() / end() (in net-next, there's no bug).
> 
> I won't comment on implications of calling dim_update_sample() in a loop.
> 
> Please make sure you answer my "did you use a tool" question, I'm really
> curious.


I have no tool, I find this when I investigating a DIM related issue.

Thanks

Re: [PATCH] virtio_net: Use u64_stats_fetch_begin() for stats fetch

[Michael S. Tsirkin]

On Wed, Jun 19, 2024 at 10:55:29AM +0800, Li RongQing wrote:
> This place is fetching the stats, so u64_stats_fetch_begin
> and u64_stats_fetch_retry should be used
> 
> Fixes: 6208799553a8 ("virtio-net: support rx netdim")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

So I dropped this from my tree, if you think it's
still necessary, pls resubmit to net-next.

Thanks!

> ---
>  drivers/net/virtio_net.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 61a57d1..b669e73 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2332,16 +2332,18 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
>  static void virtnet_rx_dim_update(struct virtnet_info *vi, struct receive_queue *rq)
>  {
>  	struct dim_sample cur_sample = {};
> +	unsigned int start;
>  
>  	if (!rq->packets_in_napi)
>  		return;
>  
> -	u64_stats_update_begin(&rq->stats.syncp);
> -	dim_update_sample(rq->calls,
> -			  u64_stats_read(&rq->stats.packets),
> -			  u64_stats_read(&rq->stats.bytes),
> -			  &cur_sample);
> -	u64_stats_update_end(&rq->stats.syncp);
> +	do {
> +		start = u64_stats_fetch_begin(&rq->stats.syncp);
> +		dim_update_sample(rq->calls,
> +				u64_stats_read(&rq->stats.packets),
> +				u64_stats_read(&rq->stats.bytes),
> +				&cur_sample);
> +	} while (u64_stats_fetch_retry(&rq->stats.syncp, start));
>  
>  	net_dim(&rq->dim, cur_sample);
>  	rq->packets_in_napi = 0;
> -- 
> 2.9.4