> On Apr 24, 2025, at 8:11 AM, Michael S. Tsirkin <m...@redhat.com> wrote:
>
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> On Thu, Apr 24, 2025 at 01:48:53PM +0200, Paolo Abeni wrote:
>> On 4/20/25 3:05 AM, Jon Kohler wrote:
>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>> index b9b9e9d40951..9b04025eea66 100644
>>> --- a/drivers/vhost/net.c
>>> +++ b/drivers/vhost/net.c
>>> @@ -769,13 +769,17 @@ static void handle_tx_copy(struct vhost_net *net,
>>> struct socket *sock)
>>> break;
>>> /* Nothing new? Wait for eventfd to tell us they refilled. */
>>> if (head == vq->num) {
>>> + /* If interrupted while doing busy polling, requeue
>>> + * the handler to be fair handle_rx as well as other
>>> + * tasks waiting on cpu
>>> + */
>>> if (unlikely(busyloop_intr)) {
>>> vhost_poll_queue(&vq->poll);
>>> - } else if (unlikely(vhost_enable_notify(&net->dev,
>>> - vq))) {
>>> - vhost_disable_notify(&net->dev, vq);
>>> - continue;
>>> }
>>> + /* Kicks are disabled at this point, break loop and
>>> + * process any remaining batched packets. Queue will
>>> + * be re-enabled afterwards.
>>> + */
>>> break;
>>> }
>>
>> It's not clear to me why the zerocopy path does not need a similar change.
>
> It can have one, it's just that Jon has a separate patch to drop
> it completely. A commit log comment mentioning this would be a good
> idea, yes.
Yea, the utility of the ZC side is a head scratcher for me, I can’t get it to
work
well to save my life. I’ve got a separate thread I need to respond to Eugenio
on, will try to circle back on that next week.
The reason this one works so well is that the last batch in the copy path can
take a non-trivial amount of time, so it opens up the guest to a real saw tooth
pattern. Getting rid of that, and all that comes with it (exits, stalls, etc),
just
pays off.
>
>>> @@ -825,7 +829,14 @@ static void handle_tx_copy(struct vhost_net *net,
>>> struct socket *sock)
>>> ++nvq->done_idx;
>>> } while (likely(!vhost_exceeds_weight(vq, ++sent_pkts, total_len)));
>>>
>>> + /* Kicks are still disabled, dispatch any remaining batched msgs. */
>>> vhost_tx_batch(net, nvq, sock, &msg);
>>> +
>>> + /* All of our work has been completed; however, before leaving the
>>> + * TX handler, do one last check for work, and requeue handler if
>>> + * necessary. If there is no work, queue will be reenabled.
>>> + */
>>> + vhost_net_busy_poll_try_queue(net, vq);
>>
>> This will call vhost_poll_queue() regardless of the 'busyloop_intr' flag
>> value, while AFAICS prior to this patch vhost_poll_queue() is only
>> performed with busyloop_intr == true. Why don't we need to take care of
>> such flag here?
>
> Hmm I agree this is worth trying, a free if possibly small performance
> gain, why not. Jon want to try?
I mentioned in the commit msg that the reason we’re doing this is to be
fair to handle_rx. If my read of vhost_net_busy_poll_try_queue is correct,
we would only call vhost_poll_queue iff:
1. The TX ring is not empty, in which case we want to run handle_tx again
2. When we go to reenable kicks, it returns non-zero, which means we
should run handle_tx again anyhow
In the ring is truly empty, and we can re-enable kicks with no drama, we
would not run vhost_poll_queue.
That said, I think what you’re saying here is, we should check the busy
flag and *not* try vhost_net_busy_poll_try_queue, right? If so, great, I did
that in an internal version of this patch; however, it adds another conditional
which for the vast majority of users is not going to add any value (I think)
Happy to dig deeper, either on this change series, or a follow up?
>
>
>> @Michael: I assume you prefer that this patch will go through the
>> net-next tree, right?
>>
>> Thanks,
>>
>> Paolo
>
> I don't mind and this seems to be what Jon wants.
> I could queue it too, but extra review it gets in the net tree is good.
My apologies, I thought all non-bug fixes had to go thru net-next,
which is why I sent the v2 to net-next; however if you want to queue
right away, I’m good with either. Its a fairly well contained patch with
a huge upside :)
>
> --
> MST
>
