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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
