[6/6] vhost_net: don't poll on -EFAULT
diff mbox series

Message ID 20120416060833.14140.28139.stgit@intel-e5620-16-2.englab.nay.redhat.com
State New, archived
Headers show
Series
  • [1/6] macvtap: zerocopy: fix offset calculation when building skb
Related show

Commit Message

Jason Wang April 16, 2012, 6:08 a.m. UTC
Currently, we restart tx polling unconditionally when sendmsg()
fails. This would cause unnecessary wakeups of vhost wokers as it's
only needed when the socket send buffer were exceeded. Fix this by
restart the tx polling only when sendmsg() returns value other than
-EFAULT.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Michael S. Tsirkin April 16, 2012, 7:16 a.m. UTC | #1
On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
> Currently, we restart tx polling unconditionally when sendmsg()
> fails. This would cause unnecessary wakeups of vhost wokers as it's
> only needed when the socket send buffer were exceeded.

Why is this a problem?

> Fix this by
> restart the tx polling only when sendmsg() returns value other than
> -EFAULT.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/vhost/net.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 29abd65..035fa95 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -262,7 +262,8 @@ static void handle_tx(struct vhost_net *net)
>  					UIO_MAXIOV;
>  			}
>  			vhost_discard_vq_desc(vq, 1);
> -			tx_poll_start(net, sock);
> +			if (err != -EFAULT)
> +				tx_poll_start(net, sock);
>  			break;
>  		}
>  		if (err != len)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jason Wang April 16, 2012, 8:28 a.m. UTC | #2
On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
>> Currently, we restart tx polling unconditionally when sendmsg()
>> fails. This would cause unnecessary wakeups of vhost wokers as it's
>> only needed when the socket send buffer were exceeded.
> Why is this a problem?

This issue is when guest driver is able to hit the -EFAULT, vhost 
discard the the descriptor and restart the polling. This would wake 
vhost thread and repeat the loop again which waste cpu.

Another possible solution is don't discard the descriptor.
>
>> Fix this by
>> restart the tx polling only when sendmsg() returns value other than
>> -EFAULT.
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> ---
>>   drivers/vhost/net.c |    3 ++-
>>   1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 29abd65..035fa95 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -262,7 +262,8 @@ static void handle_tx(struct vhost_net *net)
>>   					UIO_MAXIOV;
>>   			}
>>   			vhost_discard_vq_desc(vq, 1);
>> -			tx_poll_start(net, sock);
>> +			if (err != -EFAULT)
>> +				tx_poll_start(net, sock);
>>   			break;
>>   		}
>>   		if (err != len)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Michael S. Tsirkin April 16, 2012, 1:39 p.m. UTC | #3
On Mon, Apr 16, 2012 at 04:28:10PM +0800, Jason Wang wrote:
> On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
> >On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
> >>Currently, we restart tx polling unconditionally when sendmsg()
> >>fails. This would cause unnecessary wakeups of vhost wokers as it's
> >>only needed when the socket send buffer were exceeded.
> >Why is this a problem?
> 
> This issue is when guest driver is able to hit the -EFAULT, vhost
> discard the the descriptor and restart the polling. This would wake
> vhost thread and repeat the loop again which waste cpu.

Does same thing happen if we get an error from copy from user?

> Another possible solution is don't discard the descriptor.
> >
> >>Fix this by
> >>restart the tx polling only when sendmsg() returns value other than
> >>-EFAULT.
> >>
> >>Signed-off-by: Jason Wang<jasowang@redhat.com>
> >>---
> >>  drivers/vhost/net.c |    3 ++-
> >>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >>index 29abd65..035fa95 100644
> >>--- a/drivers/vhost/net.c
> >>+++ b/drivers/vhost/net.c
> >>@@ -262,7 +262,8 @@ static void handle_tx(struct vhost_net *net)
> >>  					UIO_MAXIOV;
> >>  			}
> >>  			vhost_discard_vq_desc(vq, 1);
> >>-			tx_poll_start(net, sock);
> >>+			if (err != -EFAULT)
> >>+				tx_poll_start(net, sock);
> >>  			break;
> >>  		}
> >>  		if (err != len)
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jason Wang April 17, 2012, 3:27 a.m. UTC | #4
On 04/16/2012 09:39 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 16, 2012 at 04:28:10PM +0800, Jason Wang wrote:
>> >  On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
>>> >  >On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
>>>> >  >>Currently, we restart tx polling unconditionally when sendmsg()
>>>> >  >>fails. This would cause unnecessary wakeups of vhost wokers as it's
>>>> >  >>only needed when the socket send buffer were exceeded.
>>> >  >Why is this a problem?
>> >  
>> >  This issue is when guest driver is able to hit the -EFAULT, vhost
>> >  discard the the descriptor and restart the polling. This would wake
>> >  vhost thread and repeat the loop again which waste cpu.
> Does same thing happen if we get an error from copy from user?
>

Right, so do you think it makes sense that we only restart polling on 
-EAGAIN or -ENOBUFS?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Michael S. Tsirkin April 17, 2012, 4:57 a.m. UTC | #5
On Tue, Apr 17, 2012 at 11:27:01AM +0800, Jason Wang wrote:
> On 04/16/2012 09:39 PM, Michael S. Tsirkin wrote:
> >On Mon, Apr 16, 2012 at 04:28:10PM +0800, Jason Wang wrote:
> >>>  On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
> >>>>  >On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
> >>>>>  >>Currently, we restart tx polling unconditionally when sendmsg()
> >>>>>  >>fails. This would cause unnecessary wakeups of vhost wokers as it's
> >>>>>  >>only needed when the socket send buffer were exceeded.
> >>>>  >Why is this a problem?
> >>>  >  This issue is when guest driver is able to hit the
> >>-EFAULT, vhost
> >>>  discard the the descriptor and restart the polling. This would wake
> >>>  vhost thread and repeat the loop again which waste cpu.
> >Does same thing happen if we get an error from copy from user?
> >
> 
> Right, so do you think it makes sense that we only restart polling
> on -EAGAIN or -ENOBUFS?

Sounds OK. BTW how do you test this?
Jason Wang April 17, 2012, 5:54 a.m. UTC | #6
On 04/17/2012 12:57 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 17, 2012 at 11:27:01AM +0800, Jason Wang wrote:
>> On 04/16/2012 09:39 PM, Michael S. Tsirkin wrote:
>>> On Mon, Apr 16, 2012 at 04:28:10PM +0800, Jason Wang wrote:
>>>>>   On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
>>>>>>   >On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
>>>>>>>   >>Currently, we restart tx polling unconditionally when sendmsg()
>>>>>>>   >>fails. This would cause unnecessary wakeups of vhost wokers as it's
>>>>>>>   >>only needed when the socket send buffer were exceeded.
>>>>>>   >Why is this a problem?
>>>>>   >   This issue is when guest driver is able to hit the
>>>> -EFAULT, vhost
>>>>>   discard the the descriptor and restart the polling. This would wake
>>>>>   vhost thread and repeat the loop again which waste cpu.
>>> Does same thing happen if we get an error from copy from user?
>>>
>> Right, so do you think it makes sense that we only restart polling
>> on -EAGAIN or -ENOBUFS?
> Sounds OK. BTW how do you test this?
>

Not very hard, w/o this patch, we can see almost 100% cpu utilization 
for vhost thread if guest hit EFAULT or EINVAL. With this patch, the cpu 
utilization should be very low I think.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Michael S. Tsirkin April 17, 2012, 6:07 a.m. UTC | #7
On Tue, Apr 17, 2012 at 01:54:55PM +0800, Jason Wang wrote:
> On 04/17/2012 12:57 PM, Michael S. Tsirkin wrote:
> >On Tue, Apr 17, 2012 at 11:27:01AM +0800, Jason Wang wrote:
> >>On 04/16/2012 09:39 PM, Michael S. Tsirkin wrote:
> >>>On Mon, Apr 16, 2012 at 04:28:10PM +0800, Jason Wang wrote:
> >>>>>  On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
> >>>>>>  >On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
> >>>>>>>  >>Currently, we restart tx polling unconditionally when sendmsg()
> >>>>>>>  >>fails. This would cause unnecessary wakeups of vhost wokers as it's
> >>>>>>>  >>only needed when the socket send buffer were exceeded.
> >>>>>>  >Why is this a problem?
> >>>>>  >   This issue is when guest driver is able to hit the
> >>>>-EFAULT, vhost
> >>>>>  discard the the descriptor and restart the polling. This would wake
> >>>>>  vhost thread and repeat the loop again which waste cpu.
> >>>Does same thing happen if we get an error from copy from user?
> >>>
> >>Right, so do you think it makes sense that we only restart polling
> >>on -EAGAIN or -ENOBUFS?
> >Sounds OK. BTW how do you test this?
> >
> 
> Not very hard, w/o this patch, we can see almost 100% cpu
> utilization for vhost thread if guest hit EFAULT or EINVAL. With
> this patch, the cpu utilization should be very low I think.

Yes but do you have a test that makes guest hit EFAULT or EINVAL?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jason Wang April 17, 2012, 6:30 a.m. UTC | #8
On 04/17/2012 02:07 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 17, 2012 at 01:54:55PM +0800, Jason Wang wrote:
>> On 04/17/2012 12:57 PM, Michael S. Tsirkin wrote:
>>> On Tue, Apr 17, 2012 at 11:27:01AM +0800, Jason Wang wrote:
>>>> On 04/16/2012 09:39 PM, Michael S. Tsirkin wrote:
>>>>> On Mon, Apr 16, 2012 at 04:28:10PM +0800, Jason Wang wrote:
>>>>>>>   On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
>>>>>>>>   >On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
>>>>>>>>>   >>Currently, we restart tx polling unconditionally when sendmsg()
>>>>>>>>>   >>fails. This would cause unnecessary wakeups of vhost wokers as it's
>>>>>>>>>   >>only needed when the socket send buffer were exceeded.
>>>>>>>>   >Why is this a problem?
>>>>>>>   >    This issue is when guest driver is able to hit the
>>>>>> -EFAULT, vhost
>>>>>>>   discard the the descriptor and restart the polling. This would wake
>>>>>>>   vhost thread and repeat the loop again which waste cpu.
>>>>> Does same thing happen if we get an error from copy from user?
>>>>>
>>>> Right, so do you think it makes sense that we only restart polling
>>>> on -EAGAIN or -ENOBUFS?
>>> Sounds OK. BTW how do you test this?
>>>
>> Not very hard, w/o this patch, we can see almost 100% cpu
>> utilization for vhost thread if guest hit EFAULT or EINVAL. With
>> this patch, the cpu utilization should be very low I think.
> Yes but do you have a test that makes guest hit EFAULT or EINVAL?

Looks like we can do this by supplying an invalid hdr_len in vnet header 
as tap does the check for this.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Michael S. Tsirkin April 17, 2012, 10:18 a.m. UTC | #9
On Tue, Apr 17, 2012 at 02:30:27PM +0800, Jason Wang wrote:
> On 04/17/2012 02:07 PM, Michael S. Tsirkin wrote:
> >On Tue, Apr 17, 2012 at 01:54:55PM +0800, Jason Wang wrote:
> >>On 04/17/2012 12:57 PM, Michael S. Tsirkin wrote:
> >>>On Tue, Apr 17, 2012 at 11:27:01AM +0800, Jason Wang wrote:
> >>>>On 04/16/2012 09:39 PM, Michael S. Tsirkin wrote:
> >>>>>On Mon, Apr 16, 2012 at 04:28:10PM +0800, Jason Wang wrote:
> >>>>>>>  On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
> >>>>>>>>  >On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
> >>>>>>>>>  >>Currently, we restart tx polling unconditionally when sendmsg()
> >>>>>>>>>  >>fails. This would cause unnecessary wakeups of vhost wokers as it's
> >>>>>>>>>  >>only needed when the socket send buffer were exceeded.
> >>>>>>>>  >Why is this a problem?
> >>>>>>>  >    This issue is when guest driver is able to hit the
> >>>>>>-EFAULT, vhost
> >>>>>>>  discard the the descriptor and restart the polling. This would wake
> >>>>>>>  vhost thread and repeat the loop again which waste cpu.
> >>>>>Does same thing happen if we get an error from copy from user?
> >>>>>
> >>>>Right, so do you think it makes sense that we only restart polling
> >>>>on -EAGAIN or -ENOBUFS?
> >>>Sounds OK. BTW how do you test this?
> >>>
> >>Not very hard, w/o this patch, we can see almost 100% cpu
> >>utilization for vhost thread if guest hit EFAULT or EINVAL. With
> >>this patch, the cpu utilization should be very low I think.
> >Yes but do you have a test that makes guest hit EFAULT or EINVAL?
> 
> Looks like we can do this by supplying an invalid hdr_len in vnet
> header as tap does the check for this.

Ah so you patched qemu to do this? Cool. Can you post the patch for testing pls?

> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jason Wang April 17, 2012, 10:46 a.m. UTC | #10
On 04/17/2012 06:18 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 17, 2012 at 02:30:27PM +0800, Jason Wang wrote:
>> >  On 04/17/2012 02:07 PM, Michael S. Tsirkin wrote:
>>> >  >On Tue, Apr 17, 2012 at 01:54:55PM +0800, Jason Wang wrote:
>>>> >  >>On 04/17/2012 12:57 PM, Michael S. Tsirkin wrote:
>>>>> >  >>>On Tue, Apr 17, 2012 at 11:27:01AM +0800, Jason Wang wrote:
>>>>>> >  >>>>On 04/16/2012 09:39 PM, Michael S. Tsirkin wrote:
>>>>>>> >  >>>>>On Mon, Apr 16, 2012 at 04:28:10PM +0800, Jason Wang wrote:
>>>>>>>>> >  >>>>>>>    On 04/16/2012 03:16 PM, Michael S. Tsirkin wrote:
>>>>>>>>>> >  >>>>>>>>    >On Mon, Apr 16, 2012 at 02:08:33PM +0800, Jason Wang wrote:
>>>>>>>>>>> >  >>>>>>>>>    >>Currently, we restart tx polling unconditionally when sendmsg()
>>>>>>>>>>> >  >>>>>>>>>    >>fails. This would cause unnecessary wakeups of vhost wokers as it's
>>>>>>>>>>> >  >>>>>>>>>    >>only needed when the socket send buffer were exceeded.
>>>>>>>>>> >  >>>>>>>>    >Why is this a problem?
>>>>>>>>> >  >>>>>>>    >     This issue is when guest driver is able to hit the
>>>>>>>> >  >>>>>>-EFAULT, vhost
>>>>>>>>> >  >>>>>>>    discard the the descriptor and restart the polling. This would wake
>>>>>>>>> >  >>>>>>>    vhost thread and repeat the loop again which waste cpu.
>>>>>>> >  >>>>>Does same thing happen if we get an error from copy from user?
>>>>>>> >  >>>>>
>>>>>> >  >>>>Right, so do you think it makes sense that we only restart polling
>>>>>> >  >>>>on -EAGAIN or -ENOBUFS?
>>>>> >  >>>Sounds OK. BTW how do you test this?
>>>>> >  >>>
>>>> >  >>Not very hard, w/o this patch, we can see almost 100% cpu
>>>> >  >>utilization for vhost thread if guest hit EFAULT or EINVAL. With
>>>> >  >>this patch, the cpu utilization should be very low I think.
>>> >  >Yes but do you have a test that makes guest hit EFAULT or EINVAL?
>> >  
>> >  Looks like we can do this by supplying an invalid hdr_len in vnet
>> >  header as tap does the check for this.
> Ah so you patched qemu to do this? Cool. Can you post the patch for testing pls?
>
No, I mean patch the guest driver like this:

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 019da01..6e2f487 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -582,7 +582,7 @@ static int xmit_skb(struct virtnet_info *vi, struct 
sk_buff *skb)
         }

         if (skb_is_gso(skb)) {
-               hdr->hdr.hdr_len = skb_headlen(skb);
+               hdr->hdr.hdr_len = 65535;
                 hdr->hdr.gso_size = skb_shinfo(skb)->gso_size;
                 if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4)
                         hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;

btw, If we still choose to drop the packet, we can hit -EFAULT by send a 
descriptor with a large number of pages.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 29abd65..035fa95 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -262,7 +262,8 @@  static void handle_tx(struct vhost_net *net)
 					UIO_MAXIOV;
 			}
 			vhost_discard_vq_desc(vq, 1);
-			tx_poll_start(net, sock);
+			if (err != -EFAULT)
+				tx_poll_start(net, sock);
 			break;
 		}
 		if (err != len)