Linux Virtualization - Jul 2018 - [PATCH net-next v5 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()

On 2018/07/04 13:31, xiangxia.m.yue at gmail.com wrote:
...
> +static void vhost_net_busy_poll(struct vhost_net *net,
> +				struct vhost_virtqueue *rvq,
> +				struct vhost_virtqueue *tvq,
> +				bool rx)
> +{
> +	unsigned long uninitialized_var(endtime);
> +	unsigned long busyloop_timeout;
> +	struct socket *sock;
> +	struct vhost_virtqueue *vq = rx ? tvq : rvq;
> +
> +	mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX:
VHOST_NET_VQ_RX);
> +
> +	vhost_disable_notify(&net->dev, vq);
> +	sock = rvq->private_data;
> +	busyloop_timeout = rx ? rvq->busyloop_timeout :
tvq->busyloop_timeout;
> +
> +	preempt_disable();
> +	endtime = busy_clock() + busyloop_timeout;
> +	while (vhost_can_busy_poll(tvq->dev, endtime) &&
> +	       !(sock && sk_has_rx_data(sock->sk)) &&
> +	       vhost_vq_avail_empty(tvq->dev, tvq))
> +		cpu_relax();
> +	preempt_enable();
> +
> +	if ((rx && !vhost_vq_avail_empty(&net->dev, vq)) ||
> +	    (!rx && (sock && sk_has_rx_data(sock->sk)))) {
> +		vhost_poll_queue(&vq->poll);
> +	} else if (vhost_enable_notify(&net->dev, vq) && rx) {
Hmm... on tx here sock has no rx data, so you are waiting for sock
wakeup for rx and vhost_enable_notify() seems not needed. Do you want
this actually?

} else if (rx && vhost_enable_notify(&net->dev, vq)) {
> +		vhost_disable_notify(&net->dev, vq);
> +		vhost_poll_queue(&vq->poll);
> +	}
-- 
Toshiaki Makita

On 2018?07?04? 15:59, Toshiaki Makita wrote:
> On 2018/07/04 13:31, xiangxia.m.yue at gmail.com wrote:
> ...
>> +static void vhost_net_busy_poll(struct vhost_net *net,
>> +				struct vhost_virtqueue *rvq,
>> +				struct vhost_virtqueue *tvq,
>> +				bool rx)
>> +{
>> +	unsigned long uninitialized_var(endtime);
>> +	unsigned long busyloop_timeout;
>> +	struct socket *sock;
>> +	struct vhost_virtqueue *vq = rx ? tvq : rvq;
>> +
>> +	mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX:
VHOST_NET_VQ_RX);
>> +
>> +	vhost_disable_notify(&net->dev, vq);
>> +	sock = rvq->private_data;
>> +	busyloop_timeout = rx ? rvq->busyloop_timeout :
tvq->busyloop_timeout;
>> +
>> +	preempt_disable();
>> +	endtime = busy_clock() + busyloop_timeout;
>> +	while (vhost_can_busy_poll(tvq->dev, endtime) &&
>> +	       !(sock && sk_has_rx_data(sock->sk)) &&
>> +	       vhost_vq_avail_empty(tvq->dev, tvq))
>> +		cpu_relax();
>> +	preempt_enable();
>> +
>> +	if ((rx && !vhost_vq_avail_empty(&net->dev, vq)) ||
>> +	    (!rx && (sock && sk_has_rx_data(sock->sk)))) {
>> +		vhost_poll_queue(&vq->poll);
>> +	} else if (vhost_enable_notify(&net->dev, vq) && rx) {
> Hmm... on tx here sock has no rx data, so you are waiting for sock
> wakeup for rx and vhost_enable_notify() seems not needed. Do you want
> this actually?
>
> } else if (rx && vhost_enable_notify(&net->dev, vq)) {
Right, rx need to be checked first here.

Thanks
>> +		vhost_disable_notify(&net->dev, vq);
>> +		vhost_poll_queue(&vq->poll);
>> +	}

On Wed, Jul 4, 2018 at 5:18 PM Jason Wang <jasowang at redhat.com>
wrote:
>
>
>
> On 2018?07?04? 15:59, Toshiaki Makita wrote:
> > On 2018/07/04 13:31, xiangxia.m.yue at gmail.com wrote:
> > ...
> >> +static void vhost_net_busy_poll(struct vhost_net *net,
> >> +                            struct vhost_virtqueue *rvq,
> >> +                            struct vhost_virtqueue *tvq,
> >> +                            bool rx)
> >> +{
> >> +    unsigned long uninitialized_var(endtime);
> >> +    unsigned long busyloop_timeout;
> >> +    struct socket *sock;
> >> +    struct vhost_virtqueue *vq = rx ? tvq : rvq;
> >> +
> >> +    mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX:
VHOST_NET_VQ_RX);
> >> +
> >> +    vhost_disable_notify(&net->dev, vq);
> >> +    sock = rvq->private_data;
> >> +    busyloop_timeout = rx ? rvq->busyloop_timeout :
tvq->busyloop_timeout;
> >> +
> >> +    preempt_disable();
> >> +    endtime = busy_clock() + busyloop_timeout;
> >> +    while (vhost_can_busy_poll(tvq->dev, endtime) &&
> >> +           !(sock && sk_has_rx_data(sock->sk))
&&
> >> +           vhost_vq_avail_empty(tvq->dev, tvq))
> >> +            cpu_relax();
> >> +    preempt_enable();
> >> +
> >> +    if ((rx && !vhost_vq_avail_empty(&net->dev,
vq)) ||
> >> +        (!rx && (sock &&
sk_has_rx_data(sock->sk)))) {
> >> +            vhost_poll_queue(&vq->poll);
> >> +    } else if (vhost_enable_notify(&net->dev, vq)
&& rx) {
> > Hmm... on tx here sock has no rx data, so you are waiting for sock
> > wakeup for rx and vhost_enable_notify() seems not needed. Do you want
> > this actually?
> >
> > } else if (rx && vhost_enable_notify(&net->dev, vq)) {
>
> Right, rx need to be checked first here.
thanks? if we dont call the vhost_enable_notify for tx. so we dont
need to call vhost_disable_notify for tx?

@@ -451,7 +451,9 @@ static void vhost_net_busy_poll(struct vhost_net *net,
                                              tvq->busyloop_timeout;

        mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX:
VHOST_NET_VQ_RX);
-       vhost_disable_notify(&net->dev, vq);
+
+       if (rx)
+               vhost_disable_notify(&net->dev, vq);

        preempt_disable();
        endtime = busy_clock() + busyloop_timeout;
> Thanks
>
> >> +            vhost_disable_notify(&net->dev, vq);
> >> +            vhost_poll_queue(&vq->poll);
> >> +    }
>