Re: [bug report] vhost_net: basic polling support

[Dan Carpenter <dan.carpenter@oracle.com>]

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