* Re: [PATCH v3 5/5] drm/cirrus: rewrite and modernize driver.
From: Noralf Trønnes @ 2019-04-05 14:43 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list,
open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE, Daniel Vetter,
Dave Airlie, sam
In-Reply-To: <20190405095219.9231-6-kraxel@redhat.com>
Den 05.04.2019 11.52, skrev Gerd Hoffmann:
> Time to kill some bad sample code people are copying from ;)
>
> This is a complete rewrite of the cirrus driver. The cirrus_mode_set()
> function is pretty much the only function which is carried over largely
> unmodified. Everything else is upside down.
>
> It is a single monster patch. But given that it does some pretty
> fundamental changes to the drivers workflow and also reduces the code
> size by roughly 70% I think it'll still be alot easier to review than a
> longish baby-step patch series.
>
> Changes summary:
> - Given the small amout of video memory (4 MB) the cirrus device has
> the rewritten driver doesn't try to manage buffers there. Instead
> it will blit (memcpy) the active framebuffer to video memory.
> - All gem objects are stored in main memory and are manged using the
> new shmem helpers. ttm is out.
> - It supports RG16, RG24 and XR24 formats. XR24 gets converted to RG24
> or RG16 at blit time if needed, to avoid the pitch becoming larger
> than what the cirrus hardware can handle.
> - The simple display pipeline is used.
> - The generic fbdev emulation is used.
> - It's a atomic driver now.
> - It runs wayland.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
Acked-by: Noralf Trønnes <noralf@tronnes.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net v6] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-04-05 21:28 UTC (permalink / raw)
To: Si-Wei Liu
Cc: jiri, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, boris.ostrovsky, davem
In-Reply-To: <1554267167-17561-1-git-send-email-si-wei.liu@oracle.com>
On Wed, Apr 03, 2019 at 12:52:47AM -0400, Si-Wei Liu wrote:
> When a netdev appears through hot plug then gets enslaved by a failover
> master that is already up and running, the slave will be opened
> right away after getting enslaved. Today there's a race that userspace
> (udev) may fail to rename the slave if the kernel (net_failover)
> opens the slave earlier than when the userspace rename happens.
> Unlike bond or team, the primary slave of failover can't be renamed by
> userspace ahead of time, since the kernel initiated auto-enslavement is
> unable to, or rather, is never meant to be synchronized with the rename
> request from userspace.
>
> As the failover slave interfaces are not designed to be operated
> directly by userspace apps: IP configuration, filter rules with
> regard to network traffic passing and etc., should all be done on master
> interface. In general, userspace apps only care about the
> name of master interface, while slave names are less important as long
> as admin users can see reliable names that may carry
> other information describing the netdev. For e.g., they can infer that
> "ens3nsby" is a standby slave of "ens3", while for a
> name like "eth0" they can't tell which master it belongs to.
>
> Historically the name of IFF_UP interface can't be changed because
> there might be admin script or management software that is already
> relying on such behavior and assumes that the slave name can't be
> changed once UP. But failover is special: with the in-kernel
> auto-enslavement mechanism, the userspace expectation for device
> enumeration and bring-up order is already broken. Previously initramfs
> and various userspace config tools were modified to bypass failover
> slaves because of auto-enslavement and duplicate MAC address. Similarly,
> in case that users care about seeing reliable slave name, the new type
> of failover slaves needs to be taken care of specifically in userspace
> anyway.
>
> It's less risky to lift up the rename restriction on failover slave
> which is already UP. Although it's possible this change may potentially
> break userspace component (most likely configuration scripts or
> management software) that assumes slave name can't be changed while
> UP, it's relatively a limited and controllable set among all userspace
> components, which can be fixed specifically to listen for the rename
> and/or link down/up events on failover slaves. Userspace component
> interacting with slaves is expected to be changed to operate on failover
> master interface instead, as the failover slave is dynamic in nature
> which may come and go at any point. The goal is to make the role of
> failover slaves less relevant, and userspace components should only
> deal with failover master in the long run.
>
> Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Stephen are you happy with this approach?
> --
> v1 -> v2:
> - Drop configurable module parameter (Sridhar)
>
> v2 -> v3:
> - Drop additional IFF_SLAVE_RENAME_OK flag (Sridhar)
> - Send down and up events around rename (Michael S. Tsirkin)
>
> v3 -> v4:
> - Simplify notification to be sent (Stephen Hemminger)
>
> v4 -> v5:
> - Sync up code with latest net-next (Sridhar)
> - Use proper structure initialization (Stephen, Jiri)
>
> v5 -> v6:
> - Make the property of live name change a generic flag (Stephen)
> ---
> include/linux/netdevice.h | 3 +++
> net/core/dev.c | 25 ++++++++++++++++++++++++-
> net/core/failover.c | 6 +++---
> 3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 78f5ec4e..ea9a63f 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1498,6 +1498,7 @@ struct net_device_ops {
> * @IFF_FAILOVER: device is a failover master device
> * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> * @IFF_L3MDEV_RX_HANDLER: only invoke the rx handler of L3 master device
> + * @IFF_LIVE_NAME_CHANGE: rename is allowed while device is running
> */
> enum netdev_priv_flags {
> IFF_802_1Q_VLAN = 1<<0,
> @@ -1530,6 +1531,7 @@ enum netdev_priv_flags {
> IFF_FAILOVER = 1<<27,
> IFF_FAILOVER_SLAVE = 1<<28,
> IFF_L3MDEV_RX_HANDLER = 1<<29,
> + IFF_LIVE_NAME_CHANGE = 1<<30,
> };
>
> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
> @@ -1561,6 +1563,7 @@ enum netdev_priv_flags {
> #define IFF_FAILOVER IFF_FAILOVER
> #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
> #define IFF_L3MDEV_RX_HANDLER IFF_L3MDEV_RX_HANDLER
> +#define IFF_LIVE_NAME_CHANGE IFF_LIVE_NAME_CHANGE
>
> /**
> * struct net_device - The DEVICE structure.
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 9823b77..48341d5 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1185,7 +1185,21 @@ int dev_change_name(struct net_device *dev, const char *newname)
> BUG_ON(!dev_net(dev));
>
> net = dev_net(dev);
> - if (dev->flags & IFF_UP)
> +
> + /* Some auto-enslaved devices e.g. failover slaves are
> + * special, as userspace might rename the device after
> + * the interface had been brought up and running since
> + * the point kernel initiated auto-enslavement. Allow
> + * live name change even when these slave devices are
> + * up and running.
> + *
> + * Typically, users of these auto-enslaving devices
> + * don't actually care about slave name change, as
> + * they are supposed to operate on master interface
> + * directly.
> + */
> + if (dev->flags & IFF_UP &&
> + likely(!(dev->priv_flags & IFF_LIVE_NAME_CHANGE)))
> return -EBUSY;
>
> write_seqcount_begin(&devnet_rename_seq);
> @@ -1232,6 +1246,15 @@ int dev_change_name(struct net_device *dev, const char *newname)
> hlist_add_head_rcu(&dev->name_hlist, dev_name_hash(net, dev->name));
> write_unlock_bh(&dev_base_lock);
>
> + if (unlikely(dev->flags & IFF_UP)) {
> + struct netdev_notifier_change_info change_info = {
> + .info.dev = dev,
> + };
> +
> + call_netdevice_notifiers_info(NETDEV_CHANGE,
> + &change_info.info);
> + }
> +
> ret = call_netdevice_notifiers(NETDEV_CHANGENAME, dev);
> ret = notifier_to_errno(ret);
>
> diff --git a/net/core/failover.c b/net/core/failover.c
> index 4a92a98..b5cd3c7 100644
> --- a/net/core/failover.c
> +++ b/net/core/failover.c
> @@ -80,14 +80,14 @@ static int failover_slave_register(struct net_device *slave_dev)
> goto err_upper_link;
> }
>
> - slave_dev->priv_flags |= IFF_FAILOVER_SLAVE;
> + slave_dev->priv_flags |= (IFF_FAILOVER_SLAVE | IFF_LIVE_NAME_CHANGE);
>
> if (fops && fops->slave_register &&
> !fops->slave_register(slave_dev, failover_dev))
> return NOTIFY_OK;
>
> netdev_upper_dev_unlink(slave_dev, failover_dev);
> - slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
> + slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_LIVE_NAME_CHANGE);
> err_upper_link:
> netdev_rx_handler_unregister(slave_dev);
> done:
> @@ -121,7 +121,7 @@ int failover_slave_unregister(struct net_device *slave_dev)
>
> netdev_rx_handler_unregister(slave_dev);
> netdev_upper_dev_unlink(slave_dev, failover_dev);
> - slave_dev->priv_flags &= ~IFF_FAILOVER_SLAVE;
> + slave_dev->priv_flags &= ~(IFF_FAILOVER_SLAVE | IFF_LIVE_NAME_CHANGE);
>
> if (fops && fops->slave_unregister &&
> !fops->slave_unregister(slave_dev, failover_dev))
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH net v6] failover: allow name change on IFF_UP slave interfaces
From: Stephen Hemminger @ 2019-04-05 21:47 UTC (permalink / raw)
To: Si-Wei Liu
Cc: jiri, mst, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, boris.ostrovsky, davem
In-Reply-To: <1554267167-17561-1-git-send-email-si-wei.liu@oracle.com>
On Wed, 3 Apr 2019 00:52:47 -0400
Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
> + if (unlikely(dev->flags & IFF_UP)) {
> + struct netdev_notifier_change_info change_info = {
> + .info.dev = dev,
> + };
> +
> + call_netdevice_notifiers_info(NETDEV_CHANGE,
> + &change_info.info);
> + }
This notifier is not really necessary, there already is a CHANGENAME
that gets sent.
NETDEV_CHANGE is used in other cases to mean that the state (flags)
have changed.
^ permalink raw reply
* Re: [PATCH net v6] failover: allow name change on IFF_UP slave interfaces
From: Stephen Hemminger @ 2019-04-05 21:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jiri, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, Si-Wei Liu, boris.ostrovsky,
davem
In-Reply-To: <20190405172835-mutt-send-email-mst@kernel.org>
On Fri, 5 Apr 2019 17:28:55 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Apr 03, 2019 at 12:52:47AM -0400, Si-Wei Liu wrote:
> > When a netdev appears through hot plug then gets enslaved by a failover
> > master that is already up and running, the slave will be opened
> > right away after getting enslaved. Today there's a race that userspace
> > (udev) may fail to rename the slave if the kernel (net_failover)
> > opens the slave earlier than when the userspace rename happens.
> > Unlike bond or team, the primary slave of failover can't be renamed by
> > userspace ahead of time, since the kernel initiated auto-enslavement is
> > unable to, or rather, is never meant to be synchronized with the rename
> > request from userspace.
> >
> > As the failover slave interfaces are not designed to be operated
> > directly by userspace apps: IP configuration, filter rules with
> > regard to network traffic passing and etc., should all be done on master
> > interface. In general, userspace apps only care about the
> > name of master interface, while slave names are less important as long
> > as admin users can see reliable names that may carry
> > other information describing the netdev. For e.g., they can infer that
> > "ens3nsby" is a standby slave of "ens3", while for a
> > name like "eth0" they can't tell which master it belongs to.
> >
> > Historically the name of IFF_UP interface can't be changed because
> > there might be admin script or management software that is already
> > relying on such behavior and assumes that the slave name can't be
> > changed once UP. But failover is special: with the in-kernel
> > auto-enslavement mechanism, the userspace expectation for device
> > enumeration and bring-up order is already broken. Previously initramfs
> > and various userspace config tools were modified to bypass failover
> > slaves because of auto-enslavement and duplicate MAC address. Similarly,
> > in case that users care about seeing reliable slave name, the new type
> > of failover slaves needs to be taken care of specifically in userspace
> > anyway.
> >
> > It's less risky to lift up the rename restriction on failover slave
> > which is already UP. Although it's possible this change may potentially
> > break userspace component (most likely configuration scripts or
> > management software) that assumes slave name can't be changed while
> > UP, it's relatively a limited and controllable set among all userspace
> > components, which can be fixed specifically to listen for the rename
> > and/or link down/up events on failover slaves. Userspace component
> > interacting with slaves is expected to be changed to operate on failover
> > master interface instead, as the failover slave is dynamic in nature
> > which may come and go at any point. The goal is to make the role of
> > failover slaves less relevant, and userspace components should only
> > deal with failover master in the long run.
> >
> > Fixes: 30c8bd5aa8b2 ("net: Introduce generic failover module")
> > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> > Reviewed-by: Liran Alon <liran.alon@oracle.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Stephen are you happy with this approach?
I think it is the best solution for what you want to do.
Did you test with some things like Free Range Routing, VPP or other userspace
control planes that consume netlink?
^ permalink raw reply
* Re: [PATCH net v6] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-04-05 22:01 UTC (permalink / raw)
To: Stephen Hemminger
Cc: jiri, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, Si-Wei Liu, boris.ostrovsky,
davem
In-Reply-To: <20190405144701.25a6b55b@shemminger-XPS-13-9360>
On Fri, Apr 05, 2019 at 02:47:01PM -0700, Stephen Hemminger wrote:
> On Wed, 3 Apr 2019 00:52:47 -0400
> Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
> >
> > + if (unlikely(dev->flags & IFF_UP)) {
> > + struct netdev_notifier_change_info change_info = {
> > + .info.dev = dev,
> > + };
> > +
> > + call_netdevice_notifiers_info(NETDEV_CHANGE,
> > + &change_info.info);
> > + }
>
> This notifier is not really necessary, there already is a CHANGENAME
> that gets sent.
> NETDEV_CHANGE is used in other cases to mean that the state (flags)
> have changed.
The point is some existing scripts might not expect name
change to happen without a status change afterwards (since it was
impossible for so long). So this reports a change
to make sure scripts do not miss it.
^ permalink raw reply
* CFP: KVM Forum 2019
From: Paolo Bonzini @ 2019-04-06 9:02 UTC (permalink / raw)
To: qemu-devel, KVM list, seabios@seabios.org, libvir-list,
linux-kernel@vger.kernel.org, Linux Virtualization
Cc: kvm-forum-2019-pc
================================================================
KVM Forum 2019: Call For Participation
October 30-November 1, 2019 - Lyon Convention Center - Lyon, France
(All submissions must be received before June 15, 2019 at 23:59 PST)
=================================================================
KVM Forum is an annual event that presents a rare opportunity
for developers and users to meet, discuss the state of Linux
virtualization technology, and plan for the challenges ahead.
We invite you to lead part of the discussion by submitting a speaking
proposal for KVM Forum 2019.
At this highly technical conference, developers driving innovation
in the KVM virtualization stack (Linux, KVM, QEMU, libvirt) can
meet users who depend on KVM as part of their offerings, or to
power their data centers and clouds.
KVM Forum will include sessions on the state of the KVM
virtualization stack, planning for the future, and many
opportunities for attendees to collaborate. As we celebrate ten years
of KVM development in the Linux kernel, KVM continues to be a
critical part of the FOSS cloud infrastructure.
This year, KVM Forum is joining Open Source Summit in Lyon,
France. Selected talks from KVM Forum will be presented on
Wednesday October 30 to the full audience of the Open Source Summit.
Also, attendees of KVM Forum will have access to all of the talks from
Open Source Summit on Wednesday.
https://events.linuxfoundation.org/events/kvm-forum-2019/program/call-for-proposals/
Suggested topics:
* Scaling, latency optimizations, performance tuning, real-time guests
* Hardening and security
* New features
* Testing
KVM and the Linux kernel:
* Nested virtualization
* Resource management (CPU, I/O, memory) and scheduling
* VFIO: IOMMU, SR-IOV, virtual GPU, etc.
* Networking: Open vSwitch, XDP, etc.
* virtio and vhost
* Architecture ports and new processor features
QEMU:
* Management interfaces: QOM and QMP
* New devices, new boards, new architectures
* Graphics, desktop virtualization and virtual GPU
* New storage features
* High availability, live migration and fault tolerance
* Emulation and TCG
* Firmware: ACPI, UEFI, coreboot, U-Boot, etc.
Management and infrastructure
* Managing KVM: Libvirt, OpenStack, oVirt, etc.
* Storage: Ceph, Gluster, SPDK, etc.r
* Network Function Virtualization: DPDK, OPNFV, OVN, etc.
* Provisioning
========================
SUBMITTING YOUR PROPOSAL
========================
Abstracts due: June 15, 2019
Please submit a short abstract (~150 words) describing your presentation
proposal. Slots vary in length up to 45 minutes.
Submit your proposal here:
https://events.linuxfoundation.org/events/kvm-forum-2019/program/call-for-proposals/
Please only use the categories "presentation" and "panel discussion"
You will receive a notification whether or not your presentation proposal
was accepted by August 12, 2019.
Speakers will receive a complimentary pass for the event. In case
your submission has multiple presenters, only the primary speaker for a
proposal will receive a complimentary event pass. For panel discussions, all
panelists will receive a complimentary event pass.
TECHNICAL TALKS
A good technical talk should not just report on what has happened over
the last year; it should present a concrete problem and how it impacts
the user and/or developer community. Whenever applicable, focus on
work that needs to be done, difficulties that haven't yet been solved,
and on decisions that other developers should be aware of. Summarizing
recent developments is okay but it should not be more than a small
portion of the overall talk.
END-USER TALKS
One of the big challenges as developers is to know what, where and how
people actually use our software. We will reserve a few slots for end
users talking about their deployment challenges and achievements.
If you are using KVM in production you are encouraged submit a speaking
proposal. Simply mark it as an end-user talk. As an end user, this is a
unique opportunity to get your input to developers.
HANDS-ON / BOF SESSIONS
We will reserve some time for people to get together and discuss
strategic decisions as well as other topics that are best solved within
smaller groups.
These sessions will be announced during the event. If you are interested
in organizing such a session, please add it to the list at
http://www.linux-kvm.org/page/KVM_Forum_2019_BOF
Let people you think who might be interested know about your BOF, and
encourage them to add their names to the wiki page as well. Please add
your ideas to the list before KVM Forum starts.
PANEL DISCUSSIONS
If you are proposing a panel discussion, please make sure that you list
all of your potential panelists in your the abstract. We will request full
biographies if a panel is accepted.
==============
HOTEL / TRAVEL
==============
This year's event will take place at the Lyon Conference Center.
For information on hotels close to the conference, please visit
https://events.linuxfoundation.org/events/kvm-forum-2019/attend/venue-travel/.
Information on conference hotel blocks will be available later in
Spring 2019.
=================
DATES TO REMEMBER
=================
Submission deadline: Saturday, June 15 at 23:59 PST
Notification: Monday, August 12
Schedule announced: Friday, August 16
Slides due date: Wednesday, October 23
Event dates: Wednesday, October 30 - Friday, November 1
Thank you for your interest in KVM. We're looking forward to your
submissions and seeing you at the KVM Forum 2019 in October!
-your KVM Forum 2019 Program Committee
Please contact us with any questions or comments at
kvm-forum-2019-pc@redhat.com
^ permalink raw reply
* Re: [PATCH] virtio-net: Remove inclusion of pci.h
From: David Miller @ 2019-04-07 1:10 UTC (permalink / raw)
To: yuval.shaia; +Cc: netdev, virtualization, mst
In-Reply-To: <20190403082045.16089-1-yuval.shaia@oracle.com>
From: Yuval Shaia <yuval.shaia@oracle.com>
Date: Wed, 3 Apr 2019 11:20:45 +0300
> This header is not in use - remove it.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
Applied to net-next
^ permalink raw reply
* Re: [PATCH] virtio-net: Fix some minor formatting errors
From: David Miller @ 2019-04-07 1:10 UTC (permalink / raw)
To: yuval.shaia; +Cc: netdev, virtualization, mst
In-Reply-To: <20190403091013.16847-1-yuval.shaia@oracle.com>
From: Yuval Shaia <yuval.shaia@oracle.com>
Date: Wed, 3 Apr 2019 12:10:13 +0300
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
Applied to net-next
^ permalink raw reply
* Re: [PATCH net v6] failover: allow name change on IFF_UP slave interfaces
From: Samudrala, Sridhar @ 2019-04-07 2:45 UTC (permalink / raw)
To: si-wei liu, Stephen Hemminger, Michael S. Tsirkin
Cc: jiri, kubakici, netdev, alexander.duyck, virtualization,
liran.alon, boris.ostrovsky, davem
In-Reply-To: <ab2443f2-11e0-4773-319a-27004520d178@oracle.com>
On 4/6/2019 12:21 AM, si-wei liu wrote:
>>>
>>> Stephen are you happy with this approach?
>> I think it is the best solution for what you want to do.
>
> Since you're asking specifically, I tried what you suggested below.
>
>> Did you test with some things like Free Range Routing,
>
> Although there might be spurious warning (which is a check for sanity
> more than an error) while slave interface is up, slave rename had been
> handled quite well there, no matter which state slave is at.
>
> https://github.com/FRRouting/frr/blob/master/zebra/if_netlink.c#L97
>
> The FRR users are supposed to operate on failover master interface
> anyway. No one is expected to configure those passive interfaces for
> routing.
>
>> VPP
> Nothing particular was seen for this one. The netlink usage there
> doesn't seem related to my change:
> https://github.com/FDio/vpp/blob/master/src/vnet/devices/netlink.c
>
>> or other userspace
>> control planes that consume netlink?
> dhcpcd (https://github.com/kobolabs/dhcpcd/blob/kobo/if-linux.c#L761)
> was tested OK.
>
> In addition, the patch seems to play quite well with systemd-udev and
> dracut/initramfs-tools. No breakage, no weird error message was seen.
>
> What else do you suggest we should try/test with?
Thanks Siwei for all the tests you are trying out. Did you notice that
any of these tests required the NETDEV_CHANGE notifier that you added?
-Sridhar
^ permalink raw reply
* Re: [PATCH net v6] failover: allow name change on IFF_UP slave interfaces
From: Stephen Hemminger @ 2019-04-07 15:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jiri, kubakici, sridhar.samudrala, alexander.duyck,
virtualization, liran.alon, netdev, Si-Wei Liu, boris.ostrovsky,
davem
In-Reply-To: <20190405175429-mutt-send-email-mst@kernel.org>
On Fri, 5 Apr 2019 18:01:43 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > This notifier is not really necessary, there already is a CHANGENAME
> > that gets sent.
> > NETDEV_CHANGE is used in other cases to mean that the state (flags)
> > have changed.
>
> The point is some existing scripts might not expect name
> change to happen without a status change afterwards (since it was
> impossible for so long). So this reports a change
> to make sure scripts do not miss it.
I don't think it matters because if device named is changed and it
is down (!IFF_UP) then only CHANGENAME is sent. The NETDEV_CHANGE is
just noise to an application.
^ permalink raw reply
* Re: [PATCH RFC 4/4] vsock/virtio: increase RX buffer size to 64 KiB
From: Jason Wang @ 2019-04-08 6:35 UTC (permalink / raw)
To: Stefan Hajnoczi, Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, Stefan Hajnoczi,
David S. Miller, Stefano Garzarella
In-Reply-To: <20190405084414.GE25152@stefanha-x1.localdomain>
On 2019/4/5 下午4:44, Stefan Hajnoczi wrote:
> On Thu, Apr 04, 2019 at 12:58:38PM +0200, Stefano Garzarella wrote:
>> In order to increase host -> guest throughput with large packets,
>> we can use 64 KiB RX buffers.
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>> include/linux/virtio_vsock.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>> index 6d7a22cc20bf..43cce304408e 100644
>> --- a/include/linux/virtio_vsock.h
>> +++ b/include/linux/virtio_vsock.h
>> @@ -10,7 +10,7 @@
>> #define VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE 128
>> #define VIRTIO_VSOCK_DEFAULT_BUF_SIZE (1024 * 256)
>> #define VIRTIO_VSOCK_DEFAULT_MAX_BUF_SIZE (1024 * 256)
>> -#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4)
>> +#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 64)
> This patch raises rx ring memory consumption from 128 * 4KB = 512KB to
> 128 * 64KB = 8MB.
>
> Michael, Jason: Any advice regarding rx/tx ring sizes and buffer sizes?
>
> Depending on rx ring size and the workload's packet size, different
> values might be preferred.
>
> This could become a tunable in the future. It determines the size of
> the guest driver's rx buffers.
In virtio-net, we have mergeable rx buffer and estimate the rx buffer
size through EWMA.
That's another reason I suggest to squash the vsock codes into virtio-net.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 0/4] vsock/virtio: optimizations to increase the throughput
From: Jason Wang @ 2019-04-08 6:43 UTC (permalink / raw)
To: Stefano Garzarella, netdev
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization,
Stefan Hajnoczi, David S. Miller
In-Reply-To: <20190404105838.101559-1-sgarzare@redhat.com>
On 2019/4/4 下午6:58, Stefano Garzarella wrote:
> This series tries to increase the throughput of virtio-vsock with slight
> changes:
> - patch 1/4: reduces the number of credit update messages sent to the
> transmitter
> - patch 2/4: allows the host to split packets on multiple buffers,
> in this way, we can remove the packet size limit to
> VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE
> - patch 3/4: uses VIRTIO_VSOCK_MAX_PKT_BUF_SIZE as the max packet size
> allowed
> - patch 4/4: increases RX buffer size to 64 KiB (affects only host->guest)
>
> RFC:
> - maybe patch 4 can be replaced with multiple queues with different
> buffer sizes or using EWMA to adapt the buffer size to the traffic
Or EWMA + mergeable rx buffer, but if we decide to unify the datapath
with virtio-net, we can reuse their codes.
>
> - as Jason suggested in a previous thread [1] I'll evaluate to use
> virtio-net as transport, but I need to understand better how to
> interface with it, maybe introducing sk_buff in virtio-vsock.
>
> Any suggestions?
My understanding is this is not a must, but if it makes things easier,
we can do this.
Another thing that may help is to implement sendpage(), which will
greatly improve the performance.
Thanks
>
> Here some benchmarks step by step. I used iperf3 [2] modified with VSOCK
> support:
>
> host -> guest [Gbps]
> pkt_size before opt. patch 1 patches 2+3 patch 4
> 64 0.060 0.102 0.102 0.096
> 256 0.22 0.40 0.40 0.36
> 512 0.42 0.82 0.85 0.74
> 1K 0.7 1.6 1.6 1.5
> 2K 1.5 3.0 3.1 2.9
> 4K 2.5 5.2 5.3 5.3
> 8K 3.9 8.4 8.6 8.8
> 16K 6.6 11.1 11.3 12.8
> 32K 9.9 15.8 15.8 18.1
> 64K 13.5 17.4 17.7 21.4
> 128K 17.9 19.0 19.0 23.6
> 256K 18.0 19.4 19.8 24.4
> 512K 18.4 19.6 20.1 25.3
>
> guest -> host [Gbps]
> pkt_size before opt. patch 1 patches 2+3
> 64 0.088 0.100 0.101
> 256 0.35 0.36 0.41
> 512 0.70 0.74 0.73
> 1K 1.1 1.3 1.3
> 2K 2.4 2.4 2.6
> 4K 4.3 4.3 4.5
> 8K 7.3 7.4 7.6
> 16K 9.2 9.6 11.1
> 32K 8.3 8.9 18.1
> 64K 8.3 8.9 25.4
> 128K 7.2 8.7 26.7
> 256K 7.7 8.4 24.9
> 512K 7.7 8.5 25.0
>
> Thanks,
> Stefano
>
> [1] https://www.spinics.net/lists/netdev/msg531783.html
> [2] https://github.com/stefano-garzarella/iperf/
>
> Stefano Garzarella (4):
> vsock/virtio: reduce credit update messages
> vhost/vsock: split packets to send using multiple buffers
> vsock/virtio: change the maximum packet size allowed
> vsock/virtio: increase RX buffer size to 64 KiB
>
> drivers/vhost/vsock.c | 35 ++++++++++++++++++++-----
> include/linux/virtio_vsock.h | 3 ++-
> net/vmw_vsock/virtio_transport_common.c | 18 +++++++++----
> 3 files changed, 44 insertions(+), 12 deletions(-)
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 0/4] vsock/virtio: optimizations to increase the throughput
From: Stefan Hajnoczi @ 2019-04-08 9:23 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
David S. Miller
In-Reply-To: <20190405074917.ooftpalmploq6x3b@steredhat>
[-- Attachment #1.1: Type: text/plain, Size: 2211 bytes --]
On Fri, Apr 05, 2019 at 09:49:17AM +0200, Stefano Garzarella wrote:
> On Thu, Apr 04, 2019 at 02:04:10PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Apr 04, 2019 at 06:47:15PM +0200, Stefano Garzarella wrote:
> > > On Thu, Apr 04, 2019 at 11:52:46AM -0400, Michael S. Tsirkin wrote:
> > > > I simply love it that you have analysed the individual impact of
> > > > each patch! Great job!
> > >
> > > Thanks! I followed Stefan's suggestions!
> > >
> > > >
> > > > For comparison's sake, it could be IMHO benefitial to add a column
> > > > with virtio-net+vhost-net performance.
> > > >
> > > > This will both give us an idea about whether the vsock layer introduces
> > > > inefficiencies, and whether the virtio-net idea has merit.
> > > >
> > >
> > > Sure, I already did TCP tests on virtio-net + vhost, starting qemu in
> > > this way:
> > > $ qemu-system-x86_64 ... \
> > > -netdev tap,id=net0,vhost=on,ifname=tap0,script=no,downscript=no \
> > > -device virtio-net-pci,netdev=net0
> > >
> > > I did also a test using TCP_NODELAY, just to be fair, because VSOCK
> > > doesn't implement something like this.
> >
> > Why not?
> >
>
> I think because originally VSOCK was designed to be simple and
> low-latency, but of course we can introduce something like that.
>
> Current implementation directly copy the buffer from the user-space in a
> virtio_vsock_pkt and enqueue it to be transmitted.
>
> Maybe we can introduce a buffer per socket where accumulate bytes and
> send it when it is full or when a timer is fired . We can also introduce
> a VSOCK_NODELAY (maybe using the same value of TCP_NODELAY for
> compatibility) to send the buffer immediately for low-latency use cases.
>
> What do you think?
Today virtio-vsock implements a 1:1 sendmsg():packet relationship
because it's simple. But there's no need for the guest to enqueue
multiple VIRTIO_VSOCK_OP_RW packets when a single large packet could
combine all payloads for a connection. This is not the same as
TCP_NODELAY but related.
I think it's worth exploring TCP_NODELAY and send_pkt_list merging.
Hopefully it won't make the code much more complicated.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 1/4] vsock/virtio: reduce credit update messages
From: Stefan Hajnoczi @ 2019-04-08 9:25 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, netdev,
David S. Miller
In-Reply-To: <20190405081648.2zflr7gxknk4q3a2@steredhat>
[-- Attachment #1.1: Type: text/plain, Size: 1321 bytes --]
On Fri, Apr 05, 2019 at 10:16:48AM +0200, Stefano Garzarella wrote:
> On Thu, Apr 04, 2019 at 08:15:39PM +0100, Stefan Hajnoczi wrote:
> > On Thu, Apr 04, 2019 at 12:58:35PM +0200, Stefano Garzarella wrote:
> > > int err = -EFAULT;
> > >
> > > spin_lock_bh(&vvs->rx_lock);
> > > @@ -288,9 +290,15 @@ virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
> > > }
> > > spin_unlock_bh(&vvs->rx_lock);
> > >
> > > - /* Send a credit pkt to peer */
> > > - virtio_transport_send_credit_update(vsk, VIRTIO_VSOCK_TYPE_STREAM,
> > > - NULL);
> > > + /* We send a credit update only when the space available seen
> > > + * by the transmitter is less than VIRTIO_VSOCK_MAX_PKT_BUF_SIZE
> > > + */
> > > + free_space = vvs->buf_alloc - (vvs->fwd_cnt - vvs->last_fwd_cnt);
> >
> > Locking? These fields should be accessed under tx_lock.
> >
>
> Yes, we need a lock, but looking in the code, vvs->fwd_cnd is written
> taking rx_lock (virtio_transport_dec_rx_pkt) and it is read with the
> tx_lock (virtio_transport_inc_tx_pkt).
>
> Maybe we should use another spin_lock shared between RX and TX for those
> fields or use atomic variables.
>
> What do you suggest?
Or make vvs->fwd_cnt atomic if it's the only field that needs to be
accessed in this manner.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 2/4] vhost/vsock: split packets to send using multiple buffers
From: Stefan Hajnoczi @ 2019-04-08 9:28 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, netdev,
David S. Miller
In-Reply-To: <20190405093608.f2zsyxnjxcba5v6r@steredhat>
[-- Attachment #1.1: Type: text/plain, Size: 1367 bytes --]
On Fri, Apr 05, 2019 at 11:36:08AM +0200, Stefano Garzarella wrote:
> On Fri, Apr 05, 2019 at 09:13:56AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Apr 04, 2019 at 12:58:36PM +0200, Stefano Garzarella wrote:
> > > - vhost_add_used(vq, head, sizeof(pkt->hdr) + pkt->len);
> > > + vhost_add_used(vq, head, sizeof(pkt->hdr) + payload_len);
> > > added = true;
> > >
> > > + pkt->off += payload_len;
> > > +
> > > + /* If we didn't send all the payload we can requeue the packet
> > > + * to send it with the next available buffer.
> > > + */
> > > + if (pkt->off < pkt->len) {
> > > + spin_lock_bh(&vsock->send_pkt_list_lock);
> > > + list_add(&pkt->list, &vsock->send_pkt_list);
> > > + spin_unlock_bh(&vsock->send_pkt_list_lock);
> > > + continue;
> >
> > The virtio_transport_deliver_tap_pkt() call is skipped. Packet capture
> > should see the exact packets that are delivered. I think this patch
> > will present one large packet instead of several smaller packets that
> > were actually delivered.
>
> I'll modify virtio_transport_build_skb() to take care of pkt->off
> and reading the payload size from the virtio_vsock_hdr.
> Otherwise, should I introduce another field in virtio_vsock_pkt to store
> the payload size?
I don't remember the details but I trust you'll pick a good way of doing
it.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 3/4] vsock/virtio: change the maximum packet size allowed
From: Stefan Hajnoczi @ 2019-04-08 9:37 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, Michael S. Tsirkin, linux-kernel, virtualization, netdev,
David S. Miller
In-Reply-To: <20190405100747.dbwi3sjaudp3d2wa@steredhat>
[-- Attachment #1.1: Type: text/plain, Size: 3545 bytes --]
On Fri, Apr 05, 2019 at 12:07:47PM +0200, Stefano Garzarella wrote:
> On Fri, Apr 05, 2019 at 09:24:47AM +0100, Stefan Hajnoczi wrote:
> > On Thu, Apr 04, 2019 at 12:58:37PM +0200, Stefano Garzarella wrote:
> > > Since now we are able to split packets, we can avoid limiting
> > > their sizes to VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE.
> > > Instead, we can use VIRTIO_VSOCK_MAX_PKT_BUF_SIZE as the max
> > > packet size.
> > >
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > > net/vmw_vsock/virtio_transport_common.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > > index f32301d823f5..822e5d07a4ec 100644
> > > --- a/net/vmw_vsock/virtio_transport_common.c
> > > +++ b/net/vmw_vsock/virtio_transport_common.c
> > > @@ -167,8 +167,8 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
> > > vvs = vsk->trans;
> > >
> > > /* we can send less than pkt_len bytes */
> > > - if (pkt_len > VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE)
> > > - pkt_len = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE;
> > > + if (pkt_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
> > > + pkt_len = VIRTIO_VSOCK_MAX_PKT_BUF_SIZE;
> >
> > The next line limits pkt_len based on available credits:
> >
> > /* virtio_transport_get_credit might return less than pkt_len credit */
> > pkt_len = virtio_transport_get_credit(vvs, pkt_len);
> >
> > I think drivers/vhost/vsock.c:vhost_transport_do_send_pkt() now works
> > correctly even with pkt_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE.
>
> Correct.
>
> >
> > The other ->send_pkt() callback is
> > net/vmw_vsock/virtio_transport.c:virtio_transport_send_pkt_work() and it
> > can already send any size packet.
> >
> > Do you remember why VIRTIO_VSOCK_MAX_PKT_BUF_SIZE still needs to be the
> > limit? I'm wondering if we can get rid of it now and just limit packets
> > to the available credits.
>
> There are 2 reasons why I left this limit:
> 1. When the host receives a packets, it must be <=
> VIRTIO_VSOCK_MAX_PKT_BUF_SIZE [drivers/vhost/vsock.c:vhost_vsock_alloc_pkt()]
> So in this way we can limit the packets sent from the guest.
The general intent is to prevent the guest from sending huge buffers.
This is good.
However, the guest must already obey the credit limit advertized by the
host. Therefore I think we should be checking against that instead of
an arbitrary constant limit.
So I think the limit should be the receive buffer size, not
VIRTIO_VSOCK_MAX_PKT_BUF_SIZE. But at this point the code doesn't know
which connection the packet is associated with and cannot check the
receive buffer size. :(
Anyway, any change to this behavior requires compatibility so new guest
drivers work with old vhost_vsock.ko. Therefore we should probably just
leave the limit for now.
> 2. When the host send packets, it help us to increase the parallelism
> (especially if the guest has 64 KB RX buffers) because the user thread
> will split packets, calling multiple times transport->stream_enqueue()
> in net/vmw_vsock/af_vsock.c:vsock_stream_sendmsg() while the
> vhost_transport_send_pkt_work() send them to the guest.
Sorry, I don't understand the reasoning. Overall this creates more
work. Are you saying the benefit is that
vhost_transport_send_pkt_work() can run "early" and notify the guest of
partial rx data before all of it has been enqueued?
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH RFC 0/4] vsock/virtio: optimizations to increase the throughput
From: Stefan Hajnoczi @ 2019-04-08 9:44 UTC (permalink / raw)
To: Jason Wang
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
David S. Miller, Stefano Garzarella
In-Reply-To: <a98d194d-e257-41ff-82e7-de2d16d2f4d4@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 383 bytes --]
On Mon, Apr 08, 2019 at 02:43:28PM +0800, Jason Wang wrote:
> Another thing that may help is to implement sendpage(), which will greatly
> improve the performance.
I can't find documentation for ->sendpage(). Is the idea that you get a
struct page for the payload and can do zero-copy tx? (And can userspace
still write to the page, invalidating checksums in the header?)
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH 01/12] virtio/s390: use vring_create_virtqueue
From: Cornelia Huck @ 2019-04-08 11:01 UTC (permalink / raw)
To: Halil Pasic
Cc: Vasily Gorbik, linux-s390, Eric Farman, Claudio Imbrenda, kvm,
Sebastian Ott, Michael S . Tsirkin, Farhan Ali, virtualization,
Martin Schwidefsky, Viktor Mihajlovski, Janosch Frank
In-Reply-To: <20190404231622.52531-2-pasic@linux.ibm.com>
On Fri, 5 Apr 2019 01:16:11 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:
> The commit 2a2d1382fe9d ("virtio: Add improved queue allocation API")
> establishes a new way of allocating virtqueues (as a part of the effort
> that taught DMA to virtio rings).
>
> In the future we will want virtio-ccw to use the DMA API as well.
>
> Let us switch from the legacy method of allocating virtqueues to
> vring_create_virtqueue() as the first step into that direction.
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> ---
> drivers/s390/virtio/virtio_ccw.c | 27 ++++++++-------------------
> 1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 74c328321889..edf4afe2d688 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -516,17 +512,10 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> err = info->num;
> goto out_err;
> }
> - size = PAGE_ALIGN(vring_size(info->num, KVM_VIRTIO_CCW_RING_ALIGN));
> - info->queue = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);
> - if (info->queue == NULL) {
> - dev_warn(&vcdev->cdev->dev, "no queue\n");
> - err = -ENOMEM;
> - goto out_err;
> - }
> + vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN,
> + vdev, true, true, ctx,
This second true means 'may_reduce_num'. Looking at the vring code, it
seems that this parameter is never checked; the code will try to
allocate a smaller queue if it can't get the requested size in any
case... this will probably be a problem for legacy virtio-pci, which
explicitly sets may_reduce_num to false. (I can try to come up with a
patch to fix that.)
> + virtio_ccw_kvm_notify, callback, name);
>
> - vq = vring_new_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, vdev,
> - true, ctx, info->queue, virtio_ccw_kvm_notify,
> - callback, name);
> if (!vq) {
> /* For now, we fail if we can't get the requested size. */
> dev_warn(&vcdev->cdev->dev, "no vq\n");
> @@ -534,15 +523,17 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> goto out_err;
> }
>
> +
Extra blank line :)
> /* Register it with the host. */
> + queue = virtqueue_get_desc_addr(vq);
> if (vcdev->revision == 0) {
> - info->info_block->l.queue = (__u64)info->queue;
> + info->info_block->l.queue = queue;
> info->info_block->l.align = KVM_VIRTIO_CCW_RING_ALIGN;
> info->info_block->l.index = i;
> info->info_block->l.num = info->num;
You always fill in the size requested by the host, but the actual size
may be smaller (see above). I don't think that is allowed for revision
0 (which implies !virtio-1). You probably need to call
vring_create_virtqueue with may_reduce_num=false for revision 0 (and
wait for the generic vring code to be fixed...)
> ccw->count = sizeof(info->info_block->l);
> } else {
> - info->info_block->s.desc = (__u64)info->queue;
> + info->info_block->s.desc = queue;
> info->info_block->s.index = i;
> info->info_block->s.num = info->num;
Here, you need to obtain the actual number via
virtqueue_get_vring_size().
> info->info_block->s.avail = (__u64)virtqueue_get_avail(vq);
^ permalink raw reply
* Re: [PATCH 00/15] Share TTM code among framebuffer drivers
From: Koenig, Christian @ 2019-04-08 11:10 UTC (permalink / raw)
To: Thomas Zimmermann, daniel@ffwll.ch, airlied@linux.ie,
kraxel@redhat.com, Huang, Ray, Zhang, Jerry, hdegoede@redhat.com,
z.liuxinliang@hisilicon.com, zourongrong@gmail.com,
kong.kongxinwei@hisilicon.com, puck.chen@hisilicon.com
Cc: dri-devel@lists.freedesktop.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <20190408092144.4548-1-tzimmermann@suse.de>
Well first problem is I'm not sure if that is a good idea. Essentially
we want to get rid of TTM in the long run.
On the other hand this work might aid with that goal, so it might be
worth a try.
Second is that this might actually not work of hand. The problem is here:
> + /* TODO: This test used to be performed by drivers, but can
> + * this actually happen? If so, should we put the check into
> + * drm_gem_ttm_of_gem()? */
> + if (!drm_is_gem_ttm(bo))
> + return;
Yeah, this test is mandatory because TTM on itself can present buffer
object which doesn't belong to the driver called.
E.g. we sometimes have BOs which don't belong to the current drivers on
a driver specific LRU. A totally brain dead design if you ask me, but
that's how it is.
Not 100% sure, but by converting all drivers to use a common GEM_TTM
backend you might actually break that test.
I'm not sure if that is actually a problem in the real world, it most
likely isn't. But I still won't bet on it without being able to test this.
Regards,
Christian.
Am 08.04.19 um 11:21 schrieb Thomas Zimmermann:
> Several simple framebuffer drivers copy most of the TTM code from each
> other. The implementation is always the same; except for the name of
> some data structures.
>
> As recently discussed, this patch set provides generic TTM memory-
> management code for framebuffers with dedicated video memory. It further
> converts the respective drivers to the generic code. The shared code
> is basically the same implementation as the one copied among individual
> drivers.
>
> The patch set contains two major changes: first, it introduces
> |struct drm_gem_ttm_object| and helpers. It's a GEM object that is
> backed by TTM-managed memory. The type's purpose is somewhat similar
> to |struct drm_gem_{cma, shmem}_object|. Second, the patch set
> introduces |struct drm_simple_ttm| (for the lack of a better name) and
> helpers. It's an implementation of a basic TTM-based memory manager.
>
> Both, GEM TTM and Simple TTM, support VRAM and SYSTEM placements. Support
> for TT could probably be added if necessary. Both can be used independedly
> from each other if desired by the DRM driver.
>
> Currently ast, bochs, mgag200, vboxvideo and hisilicon/hibmc can use
> these helpers. Cirrus would also be a candidate, but as it's being
> rewrtten from scratch, I didn't bother doing the conversion.
>
> Future directions: with these changes, the respective drivers can also
> share some of their mode-setting or fbdev code. GEM TTM could implement
> PRIME helpers, which would allow for using the generic fbcon.
>
> The patch set is against a recent drm-tip.
>
> Thomas Zimmermann (15):
> drm: Add |struct drm_gem_ttm_object| and helpers
> drm: Add |struct drm_gem_ttm_object| callbacks for |struct
> ttm_bo_driver|
> drm: Add |struct drm_gem_ttm_object| callbacks for |struct drm_driver|
> drm: Add drm_gem_ttm_fill_create_dumb() to create dumb buffers
> drm: Add Simple TTM, a memory manager for dedicated VRAM
> drm/ast: Convert AST driver to |struct drm_gem_ttm_object|
> drm/ast: Convert AST driver to Simple TTM
> drm/bochs: Convert Bochs driver to |struct drm_gem_ttm_object|
> drm/bochs: Convert Bochs driver to Simple TTM
> drm/mgag200: Convert mgag200 driver to |struct drm_gem_ttm_object|
> drm/mgag200: Convert mgag200 driver to Simple TTM
> drm/vboxvideo: Convert vboxvideo driver to |struct drm_gem_ttm_object|
> drm/vboxvideo: Convert vboxvideo driver to Simple TTM
> drm/hisilicon: Convert hibmc-drm driver to |struct drm_gem_ttm_object|
> drm/hisilicon: Convert hibmc-drm driver to Simple TTM
>
> Documentation/gpu/drm-mm.rst | 23 +
> drivers/gpu/drm/Kconfig | 20 +
> drivers/gpu/drm/Makefile | 5 +
> drivers/gpu/drm/ast/Kconfig | 3 +-
> drivers/gpu/drm/ast/ast_drv.c | 4 +-
> drivers/gpu/drm/ast/ast_drv.h | 58 +-
> drivers/gpu/drm/ast/ast_fb.c | 18 +-
> drivers/gpu/drm/ast/ast_main.c | 74 +--
> drivers/gpu/drm/ast/ast_mode.c | 78 +--
> drivers/gpu/drm/ast/ast_ttm.c | 290 +---------
> drivers/gpu/drm/bochs/Kconfig | 2 +
> drivers/gpu/drm/bochs/bochs.h | 42 +-
> drivers/gpu/drm/bochs/bochs_drv.c | 4 +-
> drivers/gpu/drm/bochs/bochs_kms.c | 18 +-
> drivers/gpu/drm/bochs/bochs_mm.c | 392 +-------------
> drivers/gpu/drm/drm_gem_ttm_helper.c | 507 ++++++++++++++++++
> drivers/gpu/drm/drm_simple_ttm_helper.c | 191 +++++++
> drivers/gpu/drm/drm_ttm_helper_common.c | 6 +
> drivers/gpu/drm/hisilicon/hibmc/Kconfig | 2 +
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 19 +-
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 4 +-
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 28 +-
> .../gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 30 +-
> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 328 +----------
> drivers/gpu/drm/mgag200/Kconfig | 2 +
> drivers/gpu/drm/mgag200/mgag200_cursor.c | 61 ++-
> drivers/gpu/drm/mgag200/mgag200_drv.c | 4 +-
> drivers/gpu/drm/mgag200/mgag200_drv.h | 68 +--
> drivers/gpu/drm/mgag200/mgag200_fb.c | 18 +-
> drivers/gpu/drm/mgag200/mgag200_main.c | 84 +--
> drivers/gpu/drm/mgag200/mgag200_mode.c | 45 +-
> drivers/gpu/drm/mgag200/mgag200_ttm.c | 290 +---------
> drivers/gpu/drm/vboxvideo/Kconfig | 2 +
> drivers/gpu/drm/vboxvideo/vbox_drv.c | 5 +-
> drivers/gpu/drm/vboxvideo/vbox_drv.h | 64 +--
> drivers/gpu/drm/vboxvideo/vbox_fb.c | 22 +-
> drivers/gpu/drm/vboxvideo/vbox_main.c | 70 +--
> drivers/gpu/drm/vboxvideo/vbox_mode.c | 36 +-
> drivers/gpu/drm/vboxvideo/vbox_ttm.c | 344 +-----------
> include/drm/drm_gem_ttm_helper.h | 119 ++++
> include/drm/drm_simple_ttm_helper.h | 74 +++
> 41 files changed, 1292 insertions(+), 2162 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c
> create mode 100644 drivers/gpu/drm/drm_simple_ttm_helper.c
> create mode 100644 drivers/gpu/drm/drm_ttm_helper_common.c
> create mode 100644 include/drm/drm_gem_ttm_helper.h
> create mode 100644 include/drm/drm_simple_ttm_helper.h
>
> --
> 2.21.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] virtio: Honour 'may_reduce_num' in vring_create_virtqueue
From: Cornelia Huck @ 2019-04-08 12:33 UTC (permalink / raw)
To: Michael S . Tsirkin, Jason Wang
Cc: kvm, Cornelia Huck, linux-kernel, stable, virtualization,
Halil Pasic
vring_create_virtqueue() allows the caller to specify via the
may_reduce_num parameter whether the vring code is allowed to
allocate a smaller ring than specified.
However, the split ring allocation code tries to allocate a
smaller ring on allocation failure regardless of what the
caller specified. This may cause trouble for e.g. virtio-pci
in legacy mode, which does not support ring resizing. (The
packed ring code does not resize in any case.)
Let's fix this by bailing out immediately in the split ring code
if the requested size cannot be allocated and may_reduce_num has
not been specified.
While at it, fix a typo in the usage instructions.
Fixes: 2a2d1382fe9d ("virtio: Add improved queue allocation API")
Cc: stable@vger.kernel.org # v4.6+
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/virtio/virtio_ring.c | 2 ++
include/linux/virtio_ring.h | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 18846afb39da..5df92c308286 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -882,6 +882,8 @@ static struct virtqueue *vring_create_virtqueue_split(
GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
if (queue)
break;
+ if (!may_reduce_num)
+ return NULL;
}
if (!num)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index fab02133a919..3dc70adfe5f5 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -63,7 +63,7 @@ struct virtqueue;
/*
* Creates a virtqueue and allocates the descriptor ring. If
* may_reduce_num is set, then this may allocate a smaller ring than
- * expected. The caller should query virtqueue_get_ring_size to learn
+ * expected. The caller should query virtqueue_get_vring_size to learn
* the actual size of the ring.
*/
struct virtqueue *vring_create_virtqueue(unsigned int index,
--
2.17.2
^ permalink raw reply related
* Re: [RFC PATCH 01/12] virtio/s390: use vring_create_virtqueue
From: Michael S. Tsirkin @ 2019-04-08 12:37 UTC (permalink / raw)
To: Cornelia Huck
Cc: Vasily Gorbik, linux-s390, Eric Farman, Claudio Imbrenda, kvm,
Sebastian Ott, Farhan Ali, virtualization, Halil Pasic,
Martin Schwidefsky, Viktor Mihajlovski, Janosch Frank
In-Reply-To: <20190408130128.7859febe.cohuck@redhat.com>
On Mon, Apr 08, 2019 at 01:01:28PM +0200, Cornelia Huck wrote:
> On Fri, 5 Apr 2019 01:16:11 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
>
> > The commit 2a2d1382fe9d ("virtio: Add improved queue allocation API")
> > establishes a new way of allocating virtqueues (as a part of the effort
> > that taught DMA to virtio rings).
> >
> > In the future we will want virtio-ccw to use the DMA API as well.
> >
> > Let us switch from the legacy method of allocating virtqueues to
> > vring_create_virtqueue() as the first step into that direction.
> >
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > ---
> > drivers/s390/virtio/virtio_ccw.c | 27 ++++++++-------------------
> > 1 file changed, 8 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> > index 74c328321889..edf4afe2d688 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
>
> > @@ -516,17 +512,10 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> > err = info->num;
> > goto out_err;
> > }
> > - size = PAGE_ALIGN(vring_size(info->num, KVM_VIRTIO_CCW_RING_ALIGN));
> > - info->queue = alloc_pages_exact(size, GFP_KERNEL | __GFP_ZERO);
> > - if (info->queue == NULL) {
> > - dev_warn(&vcdev->cdev->dev, "no queue\n");
> > - err = -ENOMEM;
> > - goto out_err;
> > - }
> > + vq = vring_create_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN,
> > + vdev, true, true, ctx,
>
> This second true means 'may_reduce_num'. Looking at the vring code, it
> seems that this parameter is never checked; the code will try to
> allocate a smaller queue if it can't get the requested size in any
> case... this will probably be a problem for legacy virtio-pci, which
> explicitly sets may_reduce_num to false. (I can try to come up with a
> patch to fix that.)
Yes, pls do. Not too late for a bugfix to go into the current linux.
> > + virtio_ccw_kvm_notify, callback, name);
> >
> > - vq = vring_new_virtqueue(i, info->num, KVM_VIRTIO_CCW_RING_ALIGN, vdev,
> > - true, ctx, info->queue, virtio_ccw_kvm_notify,
> > - callback, name);
> > if (!vq) {
> > /* For now, we fail if we can't get the requested size. */
> > dev_warn(&vcdev->cdev->dev, "no vq\n");
> > @@ -534,15 +523,17 @@ static struct virtqueue *virtio_ccw_setup_vq(struct virtio_device *vdev,
> > goto out_err;
> > }
> >
> > +
>
> Extra blank line :)
>
> > /* Register it with the host. */
> > + queue = virtqueue_get_desc_addr(vq);
> > if (vcdev->revision == 0) {
> > - info->info_block->l.queue = (__u64)info->queue;
> > + info->info_block->l.queue = queue;
> > info->info_block->l.align = KVM_VIRTIO_CCW_RING_ALIGN;
> > info->info_block->l.index = i;
> > info->info_block->l.num = info->num;
>
> You always fill in the size requested by the host, but the actual size
> may be smaller (see above). I don't think that is allowed for revision
> 0 (which implies !virtio-1). You probably need to call
> vring_create_virtqueue with may_reduce_num=false for revision 0 (and
> wait for the generic vring code to be fixed...)
>
> > ccw->count = sizeof(info->info_block->l);
> > } else {
> > - info->info_block->s.desc = (__u64)info->queue;
> > + info->info_block->s.desc = queue;
> > info->info_block->s.index = i;
> > info->info_block->s.num = info->num;
>
> Here, you need to obtain the actual number via
> virtqueue_get_vring_size().
>
> > info->info_block->s.avail = (__u64)virtqueue_get_avail(vq);
^ permalink raw reply
* Re: [PATCH] virtio: Honour 'may_reduce_num' in vring_create_virtqueue
From: Jens Freimann @ 2019-04-08 13:32 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, Michael S . Tsirkin, linux-kernel, stable, virtualization,
Halil Pasic
In-Reply-To: <20190408123322.24086-1-cohuck@redhat.com>
On Mon, Apr 08, 2019 at 02:33:22PM +0200, Cornelia Huck wrote:
>vring_create_virtqueue() allows the caller to specify via the
>may_reduce_num parameter whether the vring code is allowed to
>allocate a smaller ring than specified.
>
>However, the split ring allocation code tries to allocate a
>smaller ring on allocation failure regardless of what the
>caller specified. This may cause trouble for e.g. virtio-pci
>in legacy mode, which does not support ring resizing. (The
>packed ring code does not resize in any case.)
>
>Let's fix this by bailing out immediately in the split ring code
>if the requested size cannot be allocated and may_reduce_num has
>not been specified.
>
>While at it, fix a typo in the usage instructions.
>
>Fixes: 2a2d1382fe9d ("virtio: Add improved queue allocation API")
>Cc: stable@vger.kernel.org # v4.6+
>Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>---
> drivers/virtio/virtio_ring.c | 2 ++
> include/linux/virtio_ring.h | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
Reviewed-by: Jens Freimann <jfreimann@redhat.com>
^ permalink raw reply
* Re: [PATCH 0/2] Limit number of hw queues by nr_cpu_ids for virtio-blk and virtio-scsi
From: Dongli Zhang @ 2019-04-08 13:57 UTC (permalink / raw)
To: linux-scsi, virtualization, linux-block
Cc: axboe, martin.petersen, mst, cohuck, linux-kernel, jejb
In-Reply-To: <1553682995-5682-1-git-send-email-dongli.zhang@oracle.com>
ping?
Thank you very much!
Dongli Zhang
On 03/27/2019 06:36 PM, Dongli Zhang wrote:
> When tag_set->nr_maps is 1, the block layer limits the number of hw queues
> by nr_cpu_ids. No matter how many hw queues are use by
> virtio-blk/virtio-scsi, as they both have (tag_set->nr_maps == 1), they
> can use at most nr_cpu_ids hw queues.
>
> In addition, specifically for pci scenario, when the 'num-queues' specified
> by qemu is more than maxcpus, virtio-blk/virtio-scsi would not be able to
> allocate more than maxcpus vectors in order to have a vector for each
> queue. As a result, they fall back into MSI-X with one vector for config
> and one shared for queues.
>
> Considering above reasons, this patch set limits the number of hw queues
> used by nr_cpu_ids for both virtio-blk and virtio-scsi.
>
> -------------------------------------------------------------
>
> Here is test result of virtio-scsi:
>
> qemu cmdline:
>
> -smp 2,maxcpus=4, \
> -device virtio-scsi-pci,id=scsi0,num_queues=8, \
> -device scsi-hd,drive=drive0,bus=scsi0.0,channel=0,scsi-id=0,lun=0, \
> -drive file=test.img,if=none,id=drive0
>
> Although maxcpus=4 and num_queues=8, 4 queues are used while 2 interrupts
> are allocated.
>
> # cat /proc/interrupts
> ... ...
> 24: 0 0 PCI-MSI 65536-edge virtio0-config
> 25: 0 369 PCI-MSI 65537-edge virtio0-virtqueues
> ... ...
>
> # /sys/block/sda/mq/
> 0 1 2 3 ------> 4 queues although qemu sets num_queues=8
>
>
> With the patch set, there is per-queue interrupt.
>
> # cat /proc/interrupts
> 24: 0 0 PCI-MSI 65536-edge virtio0-config
> 25: 0 0 PCI-MSI 65537-edge virtio0-control
> 26: 0 0 PCI-MSI 65538-edge virtio0-event
> 27: 296 0 PCI-MSI 65539-edge virtio0-request
> 28: 0 139 PCI-MSI 65540-edge virtio0-request
> 29: 0 0 PCI-MSI 65541-edge virtio0-request
> 30: 0 0 PCI-MSI 65542-edge virtio0-request
>
> # ls /sys/block/sda/mq
> 0 1 2 3
>
> -------------------------------------------------------------
>
> Here is test result of virtio-blk:
>
> qemu cmdline:
>
> -smp 2,maxcpus=4,
> -device virtio-blk-pci,drive=drive-virtio-disk0,id=virtio-disk0,num-queues=8
> -drive test.img,format=raw,if=none,id=drive-virtio-disk0
>
> Although maxcpus=4 and num-queues=8, 4 queues are used while 2 interrupts
> are allocated.
>
> # cat /proc/interrupts
> ... ...
> 24: 0 0 PCI-MSI 65536-edge virtio0-config
> 25: 0 65 PCI-MSI 65537-edge virtio0-virtqueues
> ... ...
>
> # ls /sys/block/vda/mq
> 0 1 2 3 -------> 4 queues although qemu sets num_queues=8
>
>
> With the patch set, there is per-queue interrupt.
>
> # cat /proc/interrupts
> 24: 0 0 PCI-MSI 65536-edge virtio0-config
> 25: 64 0 PCI-MSI 65537-edge virtio0-req.0
> 26: 0 10290 PCI-MSI 65538-edge virtio0-req.1
> 27: 0 0 PCI-MSI 65539-edge virtio0-req.2
> 28: 0 0 PCI-MSI 65540-edge virtio0-req.3
>
> # ls /sys/block/vda/mq/
> 0 1 2 3
>
>
> Reference: https://lore.kernel.org/lkml/e4afe4c5-0262-4500-aeec-60f30734b4fc@default/
>
> Thank you very much!
>
> Dongli Zhang
>
^ permalink raw reply
* Re: [PATCH 1/1] virtio_blk: replace 0 by HCTX_TYPE_DEFAULT to index blk_mq_tag_set->map
From: Dongli Zhang @ 2019-04-08 14:00 UTC (permalink / raw)
To: virtualization, linux-block, linux-kernel; +Cc: axboe, mst
In-Reply-To: <1552354316-20204-1-git-send-email-dongli.zhang@oracle.com>
ping?
The similar patchset has been queued by linux-scsi 5.2/scsi-queue.
virtio-blk is the last place where HCTX_TYPE_DEFAULT is hardcoded.
It would be more friendly to cscope if the hardcoding is avoided.
Thank you very much!
Dongli Zhang
On 03/12/2019 09:31 AM, Dongli Zhang wrote:
> Use HCTX_TYPE_DEFAULT instead of 0 to avoid hardcoding.
>
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> ---
> This is the only place to cleanup under drivers/block/*. Another patch set
> is sent to scsi and then all of below are cleaned up:
> - block/*
> - drivers/*
>
> drivers/block/virtio_blk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4bc083b..bed6035 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -691,7 +691,8 @@ static int virtblk_map_queues(struct blk_mq_tag_set *set)
> {
> struct virtio_blk *vblk = set->driver_data;
>
> - return blk_mq_virtio_map_queues(&set->map[0], vblk->vdev, 0);
> + return blk_mq_virtio_map_queues(&set->map[HCTX_TYPE_DEFAULT],
> + vblk->vdev, 0);
> }
>
> #ifdef CONFIG_VIRTIO_BLK_SCSI
>
^ permalink raw reply
* Re: [PATCH 1/1] virtio_blk: replace 0 by HCTX_TYPE_DEFAULT to index blk_mq_tag_set->map
From: Jens Axboe @ 2019-04-08 14:08 UTC (permalink / raw)
To: Dongli Zhang, virtualization, linux-block, linux-kernel; +Cc: mst
In-Reply-To: <1552354316-20204-1-git-send-email-dongli.zhang@oracle.com>
On 3/11/19 7:31 PM, Dongli Zhang wrote:
> Use HCTX_TYPE_DEFAULT instead of 0 to avoid hardcoding.
Applied, thanks.
--
Jens Axboe
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox