* [bug report] vhost_net: basic polling support
@ 2021-08-11 12:13 Dan Carpenter
2021-08-11 12:51 ` Jason Wang
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-08-11 12:13 UTC (permalink / raw)
To: jasowang; +Cc: virtualization
Hello Jason Wang,
The patch 030881372460: "vhost_net: basic polling support" from Mar
4, 2016, leads to the following
Smatch static checker warning:
drivers/vhost/vhost.c:2565 vhost_new_msg()
warn: sleeping in atomic context
vers/vhost/net.c
509 static void vhost_net_busy_poll(struct vhost_net *net,
510 struct vhost_virtqueue *rvq,
511 struct vhost_virtqueue *tvq,
512 bool *busyloop_intr,
513 bool poll_rx)
514 {
515 unsigned long busyloop_timeout;
516 unsigned long endtime;
517 struct socket *sock;
518 struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
519
520 /* Try to hold the vq mutex of the paired virtqueue. We can't
521 * use mutex_lock() here since we could not guarantee a
522 * consistenet lock ordering.
523 */
524 if (!mutex_trylock(&vq->mutex))
525 return;
526
527 vhost_disable_notify(&net->dev, vq);
528 sock = vhost_vq_get_backend(rvq);
529
530 busyloop_timeout = poll_rx ? rvq->busyloop_timeout:
531 tvq->busyloop_timeout;
532
533 preempt_disable();
^^^^^^^^^^^^^^^^^
This bumps the preemp_count.
I guess this is to disable page faults?
534 endtime = busy_clock() + busyloop_timeout;
535
536 while (vhost_can_busy_poll(endtime)) {
537 if (vhost_has_work(&net->dev)) {
538 *busyloop_intr = true;
539 break;
540 }
541
542 if ((sock_has_rx_data(sock) &&
543 !vhost_vq_avail_empty(&net->dev, rvq)) ||
The call tree from here to the GFP_KERNEL is very long...
vhost_vq_avail_empty()
-> vhost_get_avail_idx()
-> __vhost_get_user()
-> __vhost_get_user_slow()
-> translate_desc()
-> vhost_iotlb_miss vhost_new_msg() <-- GFP_KERNEL
544 !vhost_vq_avail_empty(&net->dev, tvq))
545 break;
546
547 cpu_relax();
548 }
549
550 preempt_enable();
551
552 if (poll_rx || sock_has_rx_data(sock))
553 vhost_net_busy_poll_try_queue(net, vq);
regards,
dan carpenter
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] vhost_net: basic polling support
2021-08-11 12:13 [bug report] vhost_net: basic polling support Dan Carpenter
@ 2021-08-11 12:51 ` Jason Wang
2021-08-11 13:32 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Jason Wang @ 2021-08-11 12:51 UTC (permalink / raw)
To: Dan Carpenter; +Cc: virtualization
Hi Dan:
On Wed, Aug 11, 2021 at 8:14 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Jason Wang,
>
> The patch 030881372460: "vhost_net: basic polling support" from Mar
> 4, 2016, leads to the following
> Smatch static checker warning:
>
> drivers/vhost/vhost.c:2565 vhost_new_msg()
> warn: sleeping in atomic context
>
> vers/vhost/net.c
> 509 static void vhost_net_busy_poll(struct vhost_net *net,
> 510 struct vhost_virtqueue *rvq,
> 511 struct vhost_virtqueue *tvq,
> 512 bool *busyloop_intr,
> 513 bool poll_rx)
> 514 {
> 515 unsigned long busyloop_timeout;
> 516 unsigned long endtime;
> 517 struct socket *sock;
> 518 struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
> 519
> 520 /* Try to hold the vq mutex of the paired virtqueue. We can't
> 521 * use mutex_lock() here since we could not guarantee a
> 522 * consistenet lock ordering.
> 523 */
> 524 if (!mutex_trylock(&vq->mutex))
> 525 return;
> 526
> 527 vhost_disable_notify(&net->dev, vq);
> 528 sock = vhost_vq_get_backend(rvq);
> 529
> 530 busyloop_timeout = poll_rx ? rvq->busyloop_timeout:
> 531 tvq->busyloop_timeout;
> 532
> 533 preempt_disable();
> ^^^^^^^^^^^^^^^^^
> This bumps the preemp_count.
>
> I guess this is to disable page faults?
No, it's intended since we will use busy_clock() which uses the per
cpu variable.
>
> 534 endtime = busy_clock() + busyloop_timeout;
> 535
> 536 while (vhost_can_busy_poll(endtime)) {
> 537 if (vhost_has_work(&net->dev)) {
> 538 *busyloop_intr = true;
> 539 break;
> 540 }
> 541
> 542 if ((sock_has_rx_data(sock) &&
> 543 !vhost_vq_avail_empty(&net->dev, rvq)) ||
>
> The call tree from here to the GFP_KERNEL is very long...
>
> vhost_vq_avail_empty()
> -> vhost_get_avail_idx()
> -> __vhost_get_user()
> -> __vhost_get_user_slow()
> -> translate_desc()
> -> vhost_iotlb_miss vhost_new_msg() <-- GFP_KERNEL
This won't happen in reality since:
We will make sure the IOTLB contains the translation for avail idx.
See vq_meta_prefetch() that will be called at the begining of
handle_tx() and handle_rx().
So it looks like a false positive to me.
Thanks
>
> 544 !vhost_vq_avail_empty(&net->dev, tvq))
> 545 break;
> 546
> 547 cpu_relax();
> 548 }
> 549
> 550 preempt_enable();
> 551
> 552 if (poll_rx || sock_has_rx_data(sock))
> 553 vhost_net_busy_poll_try_queue(net, vq);
>
> regards,
> dan carpenter
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] vhost_net: basic polling support
2021-08-11 12:51 ` Jason Wang
@ 2021-08-11 13:32 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2021-08-11 13:32 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization
On Wed, Aug 11, 2021 at 08:51:22PM +0800, Jason Wang wrote:
> Hi Dan:
>
> On Wed, Aug 11, 2021 at 8:14 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Hello Jason Wang,
> >
> > The patch 030881372460: "vhost_net: basic polling support" from Mar
> > 4, 2016, leads to the following
> > Smatch static checker warning:
> >
> > drivers/vhost/vhost.c:2565 vhost_new_msg()
> > warn: sleeping in atomic context
> >
> > vers/vhost/net.c
> > 509 static void vhost_net_busy_poll(struct vhost_net *net,
> > 510 struct vhost_virtqueue *rvq,
> > 511 struct vhost_virtqueue *tvq,
> > 512 bool *busyloop_intr,
> > 513 bool poll_rx)
> > 514 {
> > 515 unsigned long busyloop_timeout;
> > 516 unsigned long endtime;
> > 517 struct socket *sock;
> > 518 struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
> > 519
> > 520 /* Try to hold the vq mutex of the paired virtqueue. We can't
> > 521 * use mutex_lock() here since we could not guarantee a
> > 522 * consistenet lock ordering.
> > 523 */
> > 524 if (!mutex_trylock(&vq->mutex))
> > 525 return;
> > 526
> > 527 vhost_disable_notify(&net->dev, vq);
> > 528 sock = vhost_vq_get_backend(rvq);
> > 529
> > 530 busyloop_timeout = poll_rx ? rvq->busyloop_timeout:
> > 531 tvq->busyloop_timeout;
> > 532
> > 533 preempt_disable();
> > ^^^^^^^^^^^^^^^^^
> > This bumps the preemp_count.
> >
> > I guess this is to disable page faults?
>
> No, it's intended since we will use busy_clock() which uses the per
> cpu variable.
>
> >
> > 534 endtime = busy_clock() + busyloop_timeout;
> > 535
> > 536 while (vhost_can_busy_poll(endtime)) {
> > 537 if (vhost_has_work(&net->dev)) {
> > 538 *busyloop_intr = true;
> > 539 break;
> > 540 }
> > 541
> > 542 if ((sock_has_rx_data(sock) &&
> > 543 !vhost_vq_avail_empty(&net->dev, rvq)) ||
> >
> > The call tree from here to the GFP_KERNEL is very long...
> >
> > vhost_vq_avail_empty()
> > -> vhost_get_avail_idx()
> > -> __vhost_get_user()
> > -> __vhost_get_user_slow()
> > -> translate_desc()
> > -> vhost_iotlb_miss vhost_new_msg() <-- GFP_KERNEL
>
> This won't happen in reality since:
>
> We will make sure the IOTLB contains the translation for avail idx.
> See vq_meta_prefetch() that will be called at the begining of
> handle_tx() and handle_rx().
>
> So it looks like a false positive to me.
Thanks for looking at this. These warnings are very complicated for
me to review because of the long call trees... Smatch is pretty good
at tracking state within a function but at the function boundaries a
lot of state is lost.
regards,
dan carpenter
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-08-11 13:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-11 12:13 [bug report] vhost_net: basic polling support Dan Carpenter
2021-08-11 12:51 ` Jason Wang
2021-08-11 13:32 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.