All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.