| Message ID | 20240619025529.5264-1-lirongqing@baidu.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio_net: Use u64_stats_fetch_begin() for stats fetch | expand |
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 >
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>
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
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
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.
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
> 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
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
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;
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(-)