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
Hi Dan:
On Wed, Aug 11, 2021 at 8:14 PM Dan Carpenter [off-list ref] 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
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 [off-list ref] wrote:
quoted
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.
quoted
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