Re: [PATCH net] Revert "vhost: cache used event for better performance"

From: Koichiro Den <den@klaipeden.com>
To: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH net] Revert "vhost: cache used event for better performance"
Date: Sun, 13 Aug 2017 23:11:00 +0900	[thread overview]
Message-ID: <1502633460.3547.1.camel@klaipeden.com> (raw)
In-Reply-To: <20170809073422-mutt-send-email-mst@kernel.org>

Thanks for your comments, Michael and Jason. And I'm sorry about late response.
To be honest, I am on a summer vacation until next Tuesday.

I noticed that what I wrote was not sufficient. Regardless of caching mechanism
existence, the "event" could legitimately be at any point out of the latest
interval, which vhost_notify checks it against, meaning that if it's out of the
interval we cannot distinguish whether or not it lags behind or has a lead. And
it seems to conform to the spec. Thanks for leading me to the spec. The corner
case I point out here is:
(0) event idx feature turned on + TX napi turned off
-> (1) guest-side TX traffic bursting occurs and delayed callback set
-> (2) some interruption triggers skb_xmit_done
-> (3) guest-side modest traffic makes the interval proceed to unbounded extent
without updating "event" since NO_INTERRUPT continues to be set on its shadow
flag.

IMHO, if you plan to make TX napi the only choice, doing this sort of
optimisation beforehand seems likely to be in vain.

So, if the none-TX napi case continues to coexist, what I would like to suggest
is not just the sort of my last email, but like making maximum staleness of
"event" less than or equal to vq.num, and afterward caching suggestion.
Otherwise, I guess I should not repost my last email since it would make matters
 too complicated even though it will soon be removed when TX-napi becomes the
only choice.

Thanks!

On Wed, 2017-08-09 at 07:37 +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 09, 2017 at 10:38:10AM +0800, Jason Wang wrote:
> > I think don't think current code can work well if vq.num is grater than
> > 2^15. Since all cached idx is u16. This looks like a bug which needs to be
> > fixed.
> 
> That's a limitation of virtio 1.0.
> 
> > > * else if the interval of vq.num is [2^15, 2^16):
> > > the logic in the original patch (809ecb9bca6a9) suffices
> > > * else (= less than 2^15) (optional):
> > > checking only (vring_need_event(vq->last_used_event, new + vq->num, new)
> > > would suffice.
> > > 
> > > Am I missing something, or is this irrelevant?
> 
> Could you pls repost the suggestion copying virtio-dev mailing list
> (subscriber only, sorry about that, but host/guest ABI discussions
> need to copy that list)?
> 
> > Looks not, I think this may work. Let me do some test.
> > 
> > Thanks
> 
> I think that at this point it's prudent to add a feature bit
> as the virtio spec does not require to never move the event index back.
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization