From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C0EE8C433DB for ; Thu, 18 Feb 2021 19:57:31 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E9F1964E0F for ; Thu, 18 Feb 2021 19:57:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E9F1964E0F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=daynix.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:33484 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lCpQX-0004Zd-LJ for qemu-devel@archiver.kernel.org; Thu, 18 Feb 2021 14:57:29 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:40828) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lCpOn-0003ES-1W for qemu-devel@nongnu.org; Thu, 18 Feb 2021 14:55:41 -0500 Received: from mail-oi1-x22b.google.com ([2607:f8b0:4864:20::22b]:40817) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lCpOk-0007YC-B4 for qemu-devel@nongnu.org; Thu, 18 Feb 2021 14:55:40 -0500 Received: by mail-oi1-x22b.google.com with SMTP id 18so3322002oiz.7 for ; Thu, 18 Feb 2021 11:55:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=daynix-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VS1Fzmr4JmJ364sFaBNaMOqu4smECu0GM4ZlmLgj/Vg=; b=TKBpZiQqPDoF08TgL7G6nVTt6d5NTDIWSTPEEJDo7q7g7+Fwm/SE8vf89kOuwbyl6O GxgKCBfn0IvLq1Wy72Ks3wKibDZRZwf78KXq9yAzezVRC8Vhd3+mm7MRmZLNompLDeK8 fuegOEWoQWGTLeLfA1PK1Hs79NtRQuBMqt1i6if8gTtniXL3uYOUUDwRSL07NNljOnpp SFlM2uW0SSLWKJGYw0Z4XhejyI9AA/11nTd5W7hqDo08AdCQZyKkELlkYxxW9QdKiAak FuCIKWxrSmsf6LxY/ot9eYUJHkbdJxJ8rnYzHbNunZ4UkKXyAEDJex81nQJqLTFY6suG rvSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VS1Fzmr4JmJ364sFaBNaMOqu4smECu0GM4ZlmLgj/Vg=; b=Ut6YsMoA53heL+dn/j2SH56Bu061xbPMLAc/AO4ZqNNQpRTzw/wn/rvbr+vnZvexRb qO5b3kGu0jgGMHlDfuNUvxtEbdVOEcnm6mKwhC832qzUpeUIOcnQs4PvQ3Tukcpy1mNG H4F/DLHY2NKZaE24wPjHq3MXu1f4VocXhetmzTjbwB4g/xCAJZD4QJRyux+sPYK2i5CK 2rJ6qz5d9zXNcu4RqRWwri9tR1Hc+2A9M6NHOfpsux6fUBc8ty7zTDQk2nSKWEIMzEIx LiJtbKgwzTRTXGHoVgJxOxgJZzwW0hsqZxS3bj/Uflr8plxzNUKT5tWQgAm6OvdPFvir miPg== X-Gm-Message-State: AOAM533diajmntl5oE7w2hOqdTPbSYhtOP3rqfTPX7N8yA4Lk0kYbPqA gdclzE8E7cpQaGH66BfVbVLn3ZBOLBDahBIS2Clg+Q== X-Google-Smtp-Source: ABdhPJzFoXtvrfEEJV6m2ZL0Q2Zo7HHdQ6/Xp6hkHgcjbVZvI4hAefz07ImhMtA7LeQr9AcgYMuQmkwK5/yaGXy0PR8= X-Received: by 2002:aca:b655:: with SMTP id g82mr4009684oif.91.1613678136548; Thu, 18 Feb 2021 11:55:36 -0800 (PST) MIME-Version: 1.0 References: <20210204202915.15925-1-yuri.benditovich@daynix.com> <20210209093201-mutt-send-email-mst@kernel.org> <20210209145105.GP1166421@redhat.com> <20210209095553-mutt-send-email-mst@kernel.org> <0890bb17-9677-ff1d-bd08-c9be791e1c81@redhat.com> In-Reply-To: From: Yuri Benditovich Date: Thu, 18 Feb 2021 21:55:25 +0200 Message-ID: Subject: Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP To: =?UTF-8?Q?Daniel_P=2E_Berrang=C3=A9?= Content-Type: multipart/alternative; boundary="0000000000001c6a2a05bba1b839" Received-SPF: none client-ip=2607:f8b0:4864:20::22b; envelope-from=yuri.benditovich@daynix.com; helo=mail-oi1-x22b.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Yan Vugenfirer , Jason Wang , qemu-devel@nongnu.org, "Michael S. Tsirkin" Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --0000000000001c6a2a05bba1b839 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Feb 18, 2021 at 11:35 AM Daniel P. Berrang=C3=A9 wrote: > On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote: > > > > On 2021/2/9 =E4=B8=8B=E5=8D=8811:04, Michael S. Tsirkin wrote: > > > On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrang=C3=A9 wro= te: > > > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote: > > > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote: > > > > > > This set of patches introduces graceful switch from tap-vhost t= o > > > > > > tap-no-vhost depending on guest features. Before that the > features > > > > > > that vhost does not support were silently cleared in > get_features. > > > > > > This creates potential problem of migration from the machine > where > > > > > > some of virtio-net features are supported by the vhost kernel t= o > the > > > > > > machine where they are not supported (packed ring as an example= ). > > > > > I still worry that adding new features will silently disable vhos= t > for people. > > > > > Can we limit the change to when a VM is migrated in? > > > > Some management applications expect bi-directional live migration t= o > > > > work, so taking specific actions on incoming migration only feels > > > > dangerous. > > > Could you be more specific? > > > > > > Bi-directional migration is currently broken > > > when migrating new kernel->old kernel. > > > > > > This seems to be the motivation for this patch, though I wish > > > it was spelled out more explicitly. > > > > > > People don't complain much, but I'm fine with fixing that > > > with a userspace fallback. > > > > > > > > > I'd rather not force the fallback on others though: vhost is generall= y > > > specified explicitly by user while features are generally set > > > automatically, so this patch will make us override what user specifie= d, > > > not nice. > > > > > > > > > > IMHO if the features we're adding cannot be expected to exist in > > > > host kernels in general, then the feature should defualt to off > > > > and require explicit user config to enable. > > > > Downstream distros which can guarantee newer kernels can flip the > > > > default in their custom machine types if they desire. > > > > > > > > Regards, > > > > Daniel > > > Unfortunately that will basically mean we are stuck with no new > features > > > for years. We did what this patch is trying to change for years now, = in > > > particular KVM also seems to happily disable CPU features not support= ed > > > by kernel so I wonder why we can't keep doing it, with tweaks for som= e > > > corner cases. > > > > > > It's probably not the corner case. > > > > So my understanding is when a feature is turned on via command line, it > > should not be cleared silently otherwise we may break migration for sur= e. > > > > E.g when packed=3Don is specified, we should disable vhost instead of > clear it > > from the device. > > If something is explicitly turned on by the user, they expect that featur= e > to be honoured, or an error to be raised. > > If something is not explicitly turned on by the user, the behaviour wrt t= he > default should be stable for any given machine type version. > > IOW, if you disable vhost by default when packed=3Don is set, then you ca= n't > later switch to letting vhost be enabled with packed=3Don, unless you tie > that change to a new machine type. > > If the user has explicitly said packed=3Don *and* vhost=3Don, then shoul= d > must honour that, or raise an error if the combination is unsupportable. > Silently disabling vhost, then vhost=3Don is not ok. > If I'm not mistaken: Inside qemu there is no possibility to determine whether the user explicitly turned vhost on. For qemu the vhost is off by default but libvirt creates a new profile with vhost on. > > Regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- > https://www.instagram.com/dberrange :| > > --0000000000001c6a2a05bba1b839 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Feb 18, 2021 at 11:35 AM Dani= el P. Berrang=C3=A9 <berrange@red= hat.com> wrote:
On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote:
>
> On 2021/2/9 =E4=B8=8B=E5=8D=8811:04, Michael S. Tsirkin wrote:
> > On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrang=C3=A9= wrote:
> > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin= wrote:
> > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditov= ich wrote:
> > > > > This set of patches introduces graceful switch fro= m tap-vhost to
> > > > > tap-no-vhost depending on guest features. Before t= hat the features
> > > > > that vhost does not support were silently cleared = in get_features.
> > > > > This creates potential problem of migration from t= he machine where
> > > > > some of virtio-net features are supported by the v= host kernel to the
> > > > > machine where they are not supported (packed ring = as an example).
> > > > I still worry that adding new features will silently di= sable vhost for people.
> > > > Can we limit the change to when a VM is migrated in? > > > Some management applications expect bi-directional live migr= ation to
> > > work, so taking specific actions on incoming migration only = feels
> > > dangerous.
> > Could you be more specific?
> >
> > Bi-directional migration is currently broken
> > when migrating new kernel->old kernel.
> >
> > This seems to be the motivation for this patch, though I wish
> > it was spelled out more explicitly.
> >
> > People don't complain much, but I'm fine with fixing that=
> > with a userspace fallback.
> >
> >
> > I'd rather not force the fallback on others though: vhost is = generally
> > specified explicitly by user while features are generally set
> > automatically, so this patch will make us override what user spec= ified,
> > not nice.
> >
> >
> > > IMHO if the features we're adding cannot be expected to = exist in
> > > host kernels in general, then the feature should defualt to = off
> > > and require explicit user config to enable.
> > > Downstream distros which can guarantee newer kernels can fli= p the
> > > default in their custom machine types if they desire.
> > >
> > > Regards,
> > > Daniel
> > Unfortunately that will basically mean we are stuck with no new f= eatures
> > for years. We did what this patch is trying to change for years n= ow, in
> > particular KVM also seems to happily disable CPU features not sup= ported
> > by kernel so I wonder why we can't keep doing it, with tweaks= for some
> > corner cases.
>
>
> It's probably not the corner case.
>
> So my understanding is when a feature is turned on via command line, i= t
> should not be cleared silently otherwise we may break migration for su= re.
>
> E.g when packed=3Don is specified, we should disable vhost instead of = clear it
> from the device.

If something is explicitly turned on by the user, they expect that feature<= br> to be honoured, or an error to be raised.

If something is not explicitly turned on by the user, the behaviour wrt the=
default should be stable for any given machine type version.

IOW, if you disable vhost by default when packed=3Don is set, then you can&= #39;t
later switch to letting vhost be enabled with packed=3Don, unless you tie that change to a new machine type.

If the user has explicitly said=C2=A0 packed=3Don *and* vhost=3Don, then sh= ould
must honour that, or raise an error if the combination is unsupportable. Silently disabling vhost, then vhost=3Don is not ok.
<= br>
If I'm not mistaken:
Inside qemu there is no po= ssibility to determine whether the user explicitly turned vhost on.
For qemu the vhost is off by default but libvirt creates a new profile w= ith vhost on.
=C2=A0

Regards,
Daniel
--
|: ht= tps://berrange.com=C2=A0 =C2=A0 =C2=A0 -o-=C2=A0 =C2=A0 h= ttps://www.flickr.com/photos/dberrange :|
|: htt= ps://libvirt.org=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0-o-=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 https://fstop138.berrange.com :|
|: https://entangle-photo.org=C2=A0 =C2=A0 -o-=C2=A0 =C2=A0 = https://www.instagram.com/dberrange :|

--0000000000001c6a2a05bba1b839--