* Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
From: Adalbert Lazăr @ 2019-03-06 9:10 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kvm, netdev, linux-kernel, virtualization, Stefan Hajnoczi,
David S . Miller, Stefano Garzarella
In-Reply-To: <20190306084104.GA22159@stefanha-x1.localdomain>
On Wed, 6 Mar 2019 08:41:04 +0000, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Lazăr wrote:
>
> Thanks for the patch, Adalbert! Please add a Signed-off-by tag so your
> patch can be merged (see Documentation/process/submitting-patches.rst
> Chapter 11 for details on the Developer's Certificate of Origin).
>
> > static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> > {
> > + const struct virtio_transport *t;
> > struct virtio_vsock_pkt_info info = {
> > .op = VIRTIO_VSOCK_OP_RST,
> > .type = le16_to_cpu(pkt->hdr.type),
> > @@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> > if (!pkt)
> > return -ENOMEM;
> >
> > - return virtio_transport_get_ops()->send_pkt(pkt);
> > + t = virtio_transport_get_ops();
> > + if (!t)
> > + return -ENOTCONN;
>
> pkt is leaked here. This is an easy mistake to make because the code is
> unclear.
Thank you for your kind words :)
> The pkt argument is the received packet that we must reply to.
> The reply packet is allocated just before line 680 and must be free
> explicitly for return -ENOTCONN.
>
> You can avoid the leak and make the code easier to read like this:
>
> struct virtio_vsock_pkt *reply;
>
> ...
>
> ------ avoid reusing 'pkt'
> v
> reply = virtio_transport_alloc_pkt(&info, 0, ...);
> if (!reply)
> return -ENOMEM;
>
> t = virtio_transport_get_ops();
> if (!t) {
> virtio_transport_free_pkt(reply); <-- prevent memory leak
> return -ENOTCONN;
> }
> return t->send_pkt(reply);
What do you think about Stefano's suggestion, to move the check above
the line were the reply is allocated?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
From: Adalbert Lazăr @ 2019-03-06 8:53 UTC (permalink / raw)
To: Stefano Garzarella
Cc: kvm, netdev, linux-kernel, virtualization, Stefan Hajnoczi,
David S . Miller
In-Reply-To: <20190306081236.mcd4o27yw67euzgv@steredhat.homenet.telecomitalia.it>
On Wed, 6 Mar 2019 09:12:36 +0100, Stefano Garzarella <sgarzare@redhat.com> wrote:
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -662,6 +662,7 @@ static int virtio_transport_reset(struct vsock_sock *vsk,
> > */
> > static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> > {
> > + const struct virtio_transport *t;
> > struct virtio_vsock_pkt_info info = {
> > .op = VIRTIO_VSOCK_OP_RST,
> > .type = le16_to_cpu(pkt->hdr.type),
> > @@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> > if (!pkt)
> > return -ENOMEM;
> >
> > - return virtio_transport_get_ops()->send_pkt(pkt);
> > + t = virtio_transport_get_ops();
> > + if (!t)
> > + return -ENOTCONN;
>
> Should be better to do this check before the virtio_transport_alloc_pkt?
>
> Otherwise, I think we should free that packet before to return -ENOTCONN.
Right! :D
I will send a second version.
^ permalink raw reply
* Re: [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
From: Stefan Hajnoczi @ 2019-03-06 8:41 UTC (permalink / raw)
To: Adalbert Lazăr
Cc: kvm, netdev, linux-kernel, virtualization, Stefan Hajnoczi,
David S . Miller, Stefano Garzarella
In-Reply-To: <20190305180145.27161-1-alazar@bitdefender.com>
[-- Attachment #1.1: Type: text/plain, Size: 1475 bytes --]
On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Lazăr wrote:
Thanks for the patch, Adalbert! Please add a Signed-off-by tag so your
patch can be merged (see Documentation/process/submitting-patches.rst
Chapter 11 for details on the Developer's Certificate of Origin).
> static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> {
> + const struct virtio_transport *t;
> struct virtio_vsock_pkt_info info = {
> .op = VIRTIO_VSOCK_OP_RST,
> .type = le16_to_cpu(pkt->hdr.type),
> @@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> if (!pkt)
> return -ENOMEM;
>
> - return virtio_transport_get_ops()->send_pkt(pkt);
> + t = virtio_transport_get_ops();
> + if (!t)
> + return -ENOTCONN;
pkt is leaked here. This is an easy mistake to make because the code is
unclear. The pkt argument is the received packet that we must reply to.
The reply packet is allocated just before line 680 and must be free
explicitly for return -ENOTCONN.
You can avoid the leak and make the code easier to read like this:
struct virtio_vsock_pkt *reply;
...
------ avoid reusing 'pkt'
v
reply = virtio_transport_alloc_pkt(&info, 0, ...);
if (!reply)
return -ENOMEM;
t = virtio_transport_get_ops();
if (!t) {
virtio_transport_free_pkt(reply); <-- prevent memory leak
return -ENOTCONN;
}
return t->send_pkt(reply);
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] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
From: Stefano Garzarella @ 2019-03-06 8:12 UTC (permalink / raw)
To: Adalbert Lazăr
Cc: kvm, netdev, linux-kernel, virtualization, Stefan Hajnoczi,
David S . Miller
In-Reply-To: <20190305180145.27161-1-alazar@bitdefender.com>
Hi Adalbert,
thanks for catching this issue, I have a comment below.
On Tue, Mar 05, 2019 at 08:01:45PM +0200, Adalbert Lazăr wrote:
> Previous to commit 22b5c0b63f32 ("vsock/virtio: fix kernel panic after device hot-unplug"),
> vsock_core_init() was called from virtio_vsock_probe(). Now,
> virtio_transport_reset_no_sock() can be called before vsock_core_init()
> has the chance to run.
>
> [Wed Feb 27 14:17:09 2019] BUG: unable to handle kernel NULL pointer dereference at 0000000000000110
> [Wed Feb 27 14:17:09 2019] #PF error: [normal kernel read fault]
> [Wed Feb 27 14:17:09 2019] PGD 0 P4D 0
> [Wed Feb 27 14:17:09 2019] Oops: 0000 [#1] SMP PTI
> [Wed Feb 27 14:17:09 2019] CPU: 3 PID: 59 Comm: kworker/3:1 Not tainted 5.0.0-rc7-390-generic-hvi #390
> [Wed Feb 27 14:17:09 2019] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
> [Wed Feb 27 14:17:09 2019] Workqueue: virtio_vsock virtio_transport_rx_work [vmw_vsock_virtio_transport]
> [Wed Feb 27 14:17:09 2019] RIP: 0010:virtio_transport_reset_no_sock+0x8c/0xc0 [vmw_vsock_virtio_transport_common]
> [Wed Feb 27 14:17:09 2019] Code: 35 8b 4f 14 48 8b 57 08 31 f6 44 8b 4f 10 44 8b 07 48 8d 7d c8 e8 84 f8 ff ff 48 85 c0 48 89 c3 74 2a e8 f7 31 03 00 48 89 df <48> 8b 80 10 01 00 00 e8 68 fb 69 ed 48 8b 75 f0 65 48 33 34 25 28
> [Wed Feb 27 14:17:09 2019] RSP: 0018:ffffb42701ab7d40 EFLAGS: 00010282
> [Wed Feb 27 14:17:09 2019] RAX: 0000000000000000 RBX: ffff9d79637ee080 RCX: 0000000000000003
> [Wed Feb 27 14:17:09 2019] RDX: 0000000000000001 RSI: 0000000000000002 RDI: ffff9d79637ee080
> [Wed Feb 27 14:17:09 2019] RBP: ffffb42701ab7d78 R08: ffff9d796fae70e0 R09: ffff9d796f403500
> [Wed Feb 27 14:17:09 2019] R10: ffffb42701ab7d90 R11: 0000000000000000 R12: ffff9d7969d09240
> [Wed Feb 27 14:17:09 2019] R13: ffff9d79624e6840 R14: ffff9d7969d09318 R15: ffff9d796d48ff80
> [Wed Feb 27 14:17:09 2019] FS: 0000000000000000(0000) GS:ffff9d796fac0000(0000) knlGS:0000000000000000
> [Wed Feb 27 14:17:09 2019] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [Wed Feb 27 14:17:09 2019] CR2: 0000000000000110 CR3: 0000000427f22000 CR4: 00000000000006e0
> [Wed Feb 27 14:17:09 2019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [Wed Feb 27 14:17:09 2019] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [Wed Feb 27 14:17:09 2019] Call Trace:
> [Wed Feb 27 14:17:09 2019] virtio_transport_recv_pkt+0x63/0x820 [vmw_vsock_virtio_transport_common]
> [Wed Feb 27 14:17:09 2019] ? kfree+0x17e/0x190
> [Wed Feb 27 14:17:09 2019] ? detach_buf_split+0x145/0x160
> [Wed Feb 27 14:17:09 2019] ? __switch_to_asm+0x40/0x70
> [Wed Feb 27 14:17:09 2019] virtio_transport_rx_work+0xa0/0x106 [vmw_vsock_virtio_transport]
> [Wed Feb 27 14:17:09 2019] NET: Registered protocol family 40
> [Wed Feb 27 14:17:09 2019] process_one_work+0x167/0x410
> [Wed Feb 27 14:17:09 2019] worker_thread+0x4d/0x460
> [Wed Feb 27 14:17:09 2019] kthread+0x105/0x140
> [Wed Feb 27 14:17:09 2019] ? rescuer_thread+0x360/0x360
> [Wed Feb 27 14:17:09 2019] ? kthread_destroy_worker+0x50/0x50
> [Wed Feb 27 14:17:09 2019] ret_from_fork+0x35/0x40
> [Wed Feb 27 14:17:09 2019] Modules linked in: vmw_vsock_virtio_transport vmw_vsock_virtio_transport_common input_leds vsock serio_raw i2c_piix4 mac_hid qemu_fw_cfg autofs4 cirrus ttm drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops virtio_net psmouse drm net_failover pata_acpi virtio_blk failover floppy
> [Wed Feb 27 14:17:09 2019] CR2: 0000000000000110
> [Wed Feb 27 14:17:09 2019] ---[ end trace baa35abd2e040fe5 ]---
> [Wed Feb 27 14:17:09 2019] RIP: 0010:virtio_transport_reset_no_sock+0x8c/0xc0 [vmw_vsock_virtio_transport_common]
> [Wed Feb 27 14:17:09 2019] Code: 35 8b 4f 14 48 8b 57 08 31 f6 44 8b 4f 10 44 8b 07 48 8d 7d c8 e8 84 f8 ff ff 48 85 c0 48 89 c3 74 2a e8 f7 31 03 00 48 89 df <48> 8b 80 10 01 00 00 e8 68 fb 69 ed 48 8b 75 f0 65 48 33 34 25 28
> [Wed Feb 27 14:17:09 2019] RSP: 0018:ffffb42701ab7d40 EFLAGS: 00010282
> [Wed Feb 27 14:17:09 2019] RAX: 0000000000000000 RBX: ffff9d79637ee080 RCX: 0000000000000003
> [Wed Feb 27 14:17:09 2019] RDX: 0000000000000001 RSI: 0000000000000002 RDI: ffff9d79637ee080
> [Wed Feb 27 14:17:09 2019] RBP: ffffb42701ab7d78 R08: ffff9d796fae70e0 R09: ffff9d796f403500
> [Wed Feb 27 14:17:09 2019] R10: ffffb42701ab7d90 R11: 0000000000000000 R12: ffff9d7969d09240
> [Wed Feb 27 14:17:09 2019] R13: ffff9d79624e6840 R14: ffff9d7969d09318 R15: ffff9d796d48ff80
> [Wed Feb 27 14:17:09 2019] FS: 0000000000000000(0000) GS:ffff9d796fac0000(0000) knlGS:0000000000000000
> [Wed Feb 27 14:17:09 2019] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [Wed Feb 27 14:17:09 2019] CR2: 0000000000000110 CR3: 0000000427f22000 CR4: 00000000000006e0
> [Wed Feb 27 14:17:09 2019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [Wed Feb 27 14:17:09 2019] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> ---
> net/vmw_vsock/virtio_transport_common.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 3ae3a33da70b..502201aaff2a 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -662,6 +662,7 @@ static int virtio_transport_reset(struct vsock_sock *vsk,
> */
> static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> {
> + const struct virtio_transport *t;
> struct virtio_vsock_pkt_info info = {
> .op = VIRTIO_VSOCK_OP_RST,
> .type = le16_to_cpu(pkt->hdr.type),
> @@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
> if (!pkt)
> return -ENOMEM;
>
> - return virtio_transport_get_ops()->send_pkt(pkt);
> + t = virtio_transport_get_ops();
> + if (!t)
> + return -ENOTCONN;
Should be better to do this check before the virtio_transport_alloc_pkt?
Otherwise, I think we should free that packet before to return -ENOTCONN.
Thanks,
Stefano
> +
> + return t->send_pkt(pkt);
> }
>
> static void virtio_transport_wait_close(struct sock *sk, long timeout)
> --
> 2.21.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-06 7:23 UTC (permalink / raw)
To: si-wei liu
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, virtualization,
liran.alon, Netdev, boris.ostrovsky, David Miller
In-Reply-To: <98243a73-a42a-6465-cf3c-a049c53b4d0c@oracle.com>
On Tue, Mar 05, 2019 at 11:15:06PM -0800, si-wei liu wrote:
>
>
> On 3/5/2019 10:43 PM, Michael S. Tsirkin wrote:
> > On Tue, Mar 05, 2019 at 04:51:00PM -0800, si-wei liu wrote:
> > >
> > > On 3/5/2019 4:36 PM, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 05, 2019 at 04:20:50PM -0800, si-wei liu wrote:
> > > > > On 3/5/2019 4:06 PM, Michael S. Tsirkin wrote:
> > > > > > On Tue, Mar 05, 2019 at 11:35:50AM -0800, si-wei liu wrote:
> > > > > > > On 3/5/2019 11:24 AM, Stephen Hemminger wrote:
> > > > > > > > On Tue, 5 Mar 2019 11:19:32 -0800
> > > > > > > > si-wei liu <si-wei.liu@oracle.com> wrote:
> > > > > > > >
> > > > > > > > > > I have a vague idea: would it work to *not* set
> > > > > > > > > > IFF_UP on slave devices at all?
> > > > > > > > > Hmm, I ever thought about this option, and it appears this solution is
> > > > > > > > > more invasive than required to convert existing scripts, despite the
> > > > > > > > > controversy of introducing internal netdev state to differentiate user
> > > > > > > > > visible state. Either we disallow slave to be brought up by user, or to
> > > > > > > > > not set IFF_UP flag but instead use the internal one, could end up with
> > > > > > > > > substantial behavioral change that breaks scripts. Consider any admin
> > > > > > > > > script that does `ip link set dev ... up' successfully just assumes the
> > > > > > > > > link is up and subsequent operation can be done as usual.
> > > > > > How would it work when carrier is off?
> > > > > >
> > > > > > > While it *may*
> > > > > > > > > work for dracut (yet to be verified), I'm a bit concerned that there are
> > > > > > > > > more scripts to be converted than those that don't follow volatile
> > > > > > > > > failover slave names. It's technically doable, but may not worth the
> > > > > > > > > effort (in terms of porting existing scripts/apps).
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > > > -Siwei
> > > > > > > > Won't work for most devices. Many devices turn off PHY and link layer
> > > > > > > > if not IFF_UP
> > > > > > > True, that's what I said about introducing internal state for those driver
> > > > > > > and other kernel component. Very invasive change indeed.
> > > > > > >
> > > > > > > -Siwei
> > > > > > Well I did say it's vague.
> > > > > > How about hiding IFF_UP from dev_get_flags (and probably
> > > > > > __dev_change_flags)?
> > > > > >
> > > > > Any different? This has small footprint for the kernel change for sure,
> > > > > while the discrepancy is still there. Anyone who writes code for IFF_UP will
> > > > > not notice IFF_FAILOVER_SLAVE.
> > > > >
> > > > > Not to mention more userspace "fixup" work has to be done due to this
> > > > > change.
> > > > >
> > > > > -Siwei
> > > > >
> > > > >
> > > > Point is it's ok since most userspace should just ignore slaves
> > > > - hopefully it will just ignore it since it already
> > > > ignores interfaces that are down.
> > > Admin script thought the interface could be bright up and do further
> > > operations without checking the UP flag.
> > These scripts then would be broken on any box with multiple interfaces
> > since not all of these would have carrier.
> Consider a script executing `ifconfig ... up' and once succeeds runs tcpdump
> or some other command relying on UP interface. It's quite common that those
> scripts don't check the UP flag but instead just rely on the well-known fact
> that the command exits with 0 meaning the interface should be UP. This
> change might well break scripts of that kind.
I am sorry I don't get it. Could you give an example
of a script that works now but would be broken?
> >
> >
> > > It doesn't look to be a reliable
> > > way of prohibit userspace from operating against slaves.
> > >
> > > -Siwei
> > >
> > >
> > This does not mean we shouldn't make an effort to disable broken
> > configurations.
> >
> > I am not arguing against your patch. Not at all. I see better
> > hiding of slaves as a separate enhancement.
> I understand, but my point is we should try to minimize unnecessary side
> impact to the current usage for whatever "hiding" effort we can make. It's
> hard to find a tradeoff sometimes.
Yes if some userspace made an assumption and it worked, we should keep
it working I think. I don't necessarily agree we should worry too much
about theoretical issues. In half a year since the feature got merged
it's unlikely there are millions of slightly different scripts using it.
> >
> >
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> Thank you.
>
> -Siwei
^ permalink raw reply
* [RFC PATCH V2 5/5] vhost: access vq metadata through kernel virtual address
From: Jason Wang @ 2019-03-06 7:18 UTC (permalink / raw)
To: jasowang, mst, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, linux-mm
In-Reply-To: <1551856692-3384-1-git-send-email-jasowang@redhat.com>
It was noticed that the copy_user() friends that was used to access
virtqueue metdata tends to be very expensive for dataplane
implementation like vhost since it involves lots of software checks,
speculation barrier, hardware feature toggling (e.g SMAP). The
extra cost will be more obvious when transferring small packets since
the time spent on metadata accessing become more significant.
This patch tries to eliminate those overheads by accessing them
through kernel virtual address by vmap(). To make the pages can be
migrated, instead of pinning them through GUP, we use MMU notifiers to
invalidate vmaps and re-establish vmaps during each round of metadata
prefetching if necessary. It looks to me .invalidate_range() is
sufficient for catching this since we don't need extra TLB flush. For
devices that doesn't use metadata prefetching, the memory accessors
fallback to normal copy_user() implementation gracefully. The
invalidation was synchronized with datapath through vq mutex, and in
order to avoid hold vq mutex during range checking, MMU notifier was
teared down when trying to modify vq metadata.
Dirty page checking is done by calling set_page_dirty_locked()
explicitly for the page that used ring stay after each round of
processing.
Note that this was only done when device IOTLB is not enabled. We
could use similar method to optimize it in the future.
Tests shows at most about 22% improvement on TX PPS when using
virtio-user + vhost_net + xdp1 + TAP on 2.6GHz Broadwell:
SMAP on | SMAP off
Before: 5.0Mpps | 6.6Mpps
After: 6.1Mpps | 7.4Mpps
Cc: <linux-mm@kvack.org>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 2 +
drivers/vhost/vhost.c | 281 +++++++++++++++++++++++++++++++++++++++++++++++++-
drivers/vhost/vhost.h | 16 +++
3 files changed, 297 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index bf55f99..c276371 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -982,6 +982,7 @@ static void handle_tx(struct vhost_net *net)
else
handle_tx_copy(net, sock);
+ vq_meta_prefetch_done(vq);
out:
mutex_unlock(&vq->mutex);
}
@@ -1250,6 +1251,7 @@ static void handle_rx(struct vhost_net *net)
vhost_net_enable_vq(net, vq);
out:
vhost_net_signal_used(nvq);
+ vq_meta_prefetch_done(vq);
mutex_unlock(&vq->mutex);
}
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1015464..36ccf7c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -434,6 +434,74 @@ static size_t vhost_get_desc_size(struct vhost_virtqueue *vq, int num)
return sizeof(*vq->desc) * num;
}
+static void vhost_uninit_vmap(struct vhost_vmap *map)
+{
+ if (map->addr) {
+ vunmap(map->unmap_addr);
+ kfree(map->pages);
+ map->pages = NULL;
+ map->npages = 0;
+ }
+
+ map->addr = NULL;
+ map->unmap_addr = NULL;
+}
+
+static void vhost_invalidate_vmap(struct vhost_virtqueue *vq,
+ struct vhost_vmap *map,
+ unsigned long ustart,
+ size_t size,
+ unsigned long start,
+ unsigned long end)
+{
+ if (end < ustart || start > ustart - 1 + size)
+ return;
+
+ dump_stack();
+ mutex_lock(&vq->mutex);
+ vhost_uninit_vmap(map);
+ mutex_unlock(&vq->mutex);
+}
+
+
+static void vhost_invalidate(struct vhost_dev *dev,
+ unsigned long start, unsigned long end)
+{
+ int i;
+
+ for (i = 0; i < dev->nvqs; i++) {
+ struct vhost_virtqueue *vq = dev->vqs[i];
+
+ vhost_invalidate_vmap(vq, &vq->avail_ring,
+ (unsigned long)vq->avail,
+ vhost_get_avail_size(vq, vq->num),
+ start, end);
+ vhost_invalidate_vmap(vq, &vq->desc_ring,
+ (unsigned long)vq->desc,
+ vhost_get_desc_size(vq, vq->num),
+ start, end);
+ vhost_invalidate_vmap(vq, &vq->used_ring,
+ (unsigned long)vq->used,
+ vhost_get_used_size(vq, vq->num),
+ start, end);
+ }
+}
+
+
+static void vhost_invalidate_range(struct mmu_notifier *mn,
+ struct mm_struct *mm,
+ unsigned long start, unsigned long end)
+{
+ struct vhost_dev *dev = container_of(mn, struct vhost_dev,
+ mmu_notifier);
+
+ vhost_invalidate(dev, start, end);
+}
+
+static const struct mmu_notifier_ops vhost_mmu_notifier_ops = {
+ .invalidate_range = vhost_invalidate_range,
+};
+
void vhost_dev_init(struct vhost_dev *dev,
struct vhost_virtqueue **vqs, int nvqs, int iov_limit)
{
@@ -449,6 +517,7 @@ void vhost_dev_init(struct vhost_dev *dev,
dev->mm = NULL;
dev->worker = NULL;
dev->iov_limit = iov_limit;
+ dev->mmu_notifier.ops = &vhost_mmu_notifier_ops;
init_llist_head(&dev->work_list);
init_waitqueue_head(&dev->wait);
INIT_LIST_HEAD(&dev->read_list);
@@ -462,6 +531,9 @@ void vhost_dev_init(struct vhost_dev *dev,
vq->indirect = NULL;
vq->heads = NULL;
vq->dev = dev;
+ vq->avail_ring.addr = NULL;
+ vq->used_ring.addr = NULL;
+ vq->desc_ring.addr = NULL;
mutex_init(&vq->mutex);
vhost_vq_reset(dev, vq);
if (vq->handle_kick)
@@ -542,7 +614,13 @@ long vhost_dev_set_owner(struct vhost_dev *dev)
if (err)
goto err_cgroup;
+ err = mmu_notifier_register(&dev->mmu_notifier, dev->mm);
+ if (err)
+ goto err_mmu_notifier;
+
return 0;
+err_mmu_notifier:
+ vhost_dev_free_iovecs(dev);
err_cgroup:
kthread_stop(worker);
dev->worker = NULL;
@@ -633,6 +711,81 @@ static void vhost_clear_msg(struct vhost_dev *dev)
spin_unlock(&dev->iotlb_lock);
}
+static int vhost_init_vmap(struct vhost_dev *dev,
+ struct vhost_vmap *map, unsigned long uaddr,
+ size_t size, int write)
+{
+ struct page **pages;
+ int npages = DIV_ROUND_UP(size, PAGE_SIZE);
+ int npinned;
+ void *vaddr;
+ int err = -EFAULT;
+
+ err = -ENOMEM;
+ pages = kmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
+ if (!pages)
+ goto err_uaddr;
+
+ err = EFAULT;
+ npinned = get_user_pages_fast(uaddr, npages, write, pages);
+ if (npinned != npages)
+ goto err_gup;
+
+ vaddr = vmap(pages, npages, VM_MAP, PAGE_KERNEL);
+ if (!vaddr)
+ goto err_gup;
+
+ map->addr = vaddr + (uaddr & (PAGE_SIZE - 1));
+ map->unmap_addr = vaddr;
+ map->npages = npages;
+ map->pages = pages;
+
+err_gup:
+ /* Don't pin pages, mmu notifier will notify us about page
+ * migration.
+ */
+ if (npinned > 0)
+ release_pages(pages, npinned);
+err_uaddr:
+ return err;
+}
+
+static void vhost_uninit_vq_vmaps(struct vhost_virtqueue *vq)
+{
+ vhost_uninit_vmap(&vq->avail_ring);
+ vhost_uninit_vmap(&vq->desc_ring);
+ vhost_uninit_vmap(&vq->used_ring);
+}
+
+static int vhost_setup_avail_vmap(struct vhost_virtqueue *vq,
+ unsigned long avail)
+{
+ return vhost_init_vmap(vq->dev, &vq->avail_ring, avail,
+ vhost_get_avail_size(vq, vq->num), false);
+}
+
+static int vhost_setup_desc_vmap(struct vhost_virtqueue *vq,
+ unsigned long desc)
+{
+ return vhost_init_vmap(vq->dev, &vq->desc_ring, desc,
+ vhost_get_desc_size(vq, vq->num), false);
+}
+
+static int vhost_setup_used_vmap(struct vhost_virtqueue *vq,
+ unsigned long used)
+{
+ return vhost_init_vmap(vq->dev, &vq->used_ring, used,
+ vhost_get_used_size(vq, vq->num), true);
+}
+
+static void vhost_set_vmap_dirty(struct vhost_vmap *used)
+{
+ int i;
+
+ for (i = 0; i < used->npages; i++)
+ set_page_dirty_lock(used->pages[i]);
+}
+
void vhost_dev_cleanup(struct vhost_dev *dev)
{
int i;
@@ -662,8 +815,12 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
kthread_stop(dev->worker);
dev->worker = NULL;
}
- if (dev->mm)
+ for (i = 0; i < dev->nvqs; i++)
+ vhost_uninit_vq_vmaps(dev->vqs[i]);
+ if (dev->mm) {
+ mmu_notifier_unregister(&dev->mmu_notifier, dev->mm);
mmput(dev->mm);
+ }
dev->mm = NULL;
}
EXPORT_SYMBOL_GPL(vhost_dev_cleanup);
@@ -892,6 +1049,16 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ *((__virtio16 *)&used->ring[vq->num]) =
+ cpu_to_vhost16(vq, vq->avail_idx);
+ return 0;
+ }
+ }
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
vhost_avail_event(vq));
}
@@ -900,6 +1067,16 @@ static inline int vhost_put_used(struct vhost_virtqueue *vq,
struct vring_used_elem *head, int idx,
int count)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ memcpy(used->ring + idx, head,
+ count * sizeof(*head));
+ return 0;
+ }
+ }
+
return vhost_copy_to_user(vq, vq->used->ring + idx, head,
count * sizeof(*head));
}
@@ -907,6 +1084,15 @@ static inline int vhost_put_used(struct vhost_virtqueue *vq,
static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ used->flags = cpu_to_vhost16(vq, vq->used_flags);
+ return 0;
+ }
+ }
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
&vq->used->flags);
}
@@ -914,6 +1100,15 @@ static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ used->idx = cpu_to_vhost16(vq, vq->last_used_idx);
+ return 0;
+ }
+ }
+
return vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
&vq->used->idx);
}
@@ -959,12 +1154,30 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d)
static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,
__virtio16 *idx)
{
+ if (!vq->iotlb) {
+ struct vring_avail *avail = vq->avail_ring.addr;
+
+ if (likely(avail)) {
+ *idx = avail->idx;
+ return 0;
+ }
+ }
+
return vhost_get_avail(vq, *idx, &vq->avail->idx);
}
static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
__virtio16 *head, int idx)
{
+ if (!vq->iotlb) {
+ struct vring_avail *avail = vq->avail_ring.addr;
+
+ if (likely(avail)) {
+ *head = avail->ring[idx & (vq->num - 1)];
+ return 0;
+ }
+ }
+
return vhost_get_avail(vq, *head,
&vq->avail->ring[idx & (vq->num - 1)]);
}
@@ -972,24 +1185,60 @@ static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
__virtio16 *flags)
{
+ if (!vq->iotlb) {
+ struct vring_avail *avail = vq->avail_ring.addr;
+
+ if (likely(avail)) {
+ *flags = avail->flags;
+ return 0;
+ }
+ }
+
return vhost_get_avail(vq, *flags, &vq->avail->flags);
}
static inline int vhost_get_used_event(struct vhost_virtqueue *vq,
__virtio16 *event)
{
+ if (!vq->iotlb) {
+ struct vring_avail *avail = vq->avail_ring.addr;
+
+ if (likely(avail)) {
+ *event = (__virtio16)avail->ring[vq->num];
+ return 0;
+ }
+ }
+
return vhost_get_avail(vq, *event, vhost_used_event(vq));
}
static inline int vhost_get_used_idx(struct vhost_virtqueue *vq,
__virtio16 *idx)
{
+ if (!vq->iotlb) {
+ struct vring_used *used = vq->used_ring.addr;
+
+ if (likely(used)) {
+ *idx = used->idx;
+ return 0;
+ }
+ }
+
return vhost_get_used(vq, *idx, &vq->used->idx);
}
static inline int vhost_get_desc(struct vhost_virtqueue *vq,
struct vring_desc *desc, int idx)
{
+ if (!vq->iotlb) {
+ struct vring_desc *d = vq->desc_ring.addr;
+
+ if (likely(d)) {
+ *desc = *(d + idx);
+ return 0;
+ }
+ }
+
return vhost_copy_from_user(vq, desc, vq->desc + idx, sizeof(*desc));
}
@@ -1330,8 +1579,16 @@ int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
unsigned int num = vq->num;
- if (!vq->iotlb)
+ if (!vq->iotlb) {
+ if (unlikely(!vq->avail_ring.addr))
+ vhost_setup_avail_vmap(vq, (unsigned long)vq->avail);
+ if (unlikely(!vq->desc_ring.addr))
+ vhost_setup_desc_vmap(vq, (unsigned long)vq->desc);
+ if (unlikely(!vq->used_ring.addr))
+ vhost_setup_used_vmap(vq, (unsigned long)vq->used);
+
return 1;
+ }
return iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->desc,
vhost_get_desc_size(vq, num), VHOST_ADDR_DESC) &&
@@ -1343,6 +1600,15 @@ int vq_meta_prefetch(struct vhost_virtqueue *vq)
}
EXPORT_SYMBOL_GPL(vq_meta_prefetch);
+void vq_meta_prefetch_done(struct vhost_virtqueue *vq)
+{
+ if (vq->iotlb)
+ return;
+ if (likely(vq->used_ring.addr))
+ vhost_set_vmap_dirty(&vq->used_ring);
+}
+EXPORT_SYMBOL_GPL(vq_meta_prefetch_done);
+
/* Can we log writes? */
/* Caller should have device mutex but not vq mutex */
bool vhost_log_access_ok(struct vhost_dev *dev)
@@ -1483,6 +1749,13 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
mutex_lock(&vq->mutex);
+ /* Unregister MMU notifer to allow invalidation callback
+ * can access vq->avail, vq->desc , vq->used and vq->num
+ * without holding vq->mutex.
+ */
+ if (d->mm)
+ mmu_notifier_unregister(&d->mmu_notifier, d->mm);
+
switch (ioctl) {
case VHOST_SET_VRING_NUM:
/* Resizing ring with an active backend?
@@ -1499,6 +1772,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
r = -EINVAL;
break;
}
+ vhost_uninit_vq_vmaps(vq);
vq->num = s.num;
break;
case VHOST_SET_VRING_BASE:
@@ -1581,6 +1855,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
vq->log_addr = a.log_guest_addr;
vq->used = (void __user *)(unsigned long)a.used_user_addr;
+ vhost_uninit_vq_vmaps(vq);
break;
case VHOST_SET_VRING_KICK:
if (copy_from_user(&f, argp, sizeof f)) {
@@ -1656,6 +1931,8 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
if (pollstart && vq->handle_kick)
r = vhost_poll_start(&vq->poll, vq->kick);
+ if (d->mm)
+ mmu_notifier_register(&d->mmu_notifier, d->mm);
mutex_unlock(&vq->mutex);
if (pollstop && vq->handle_kick)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 7a7fc00..146076e 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -12,6 +12,8 @@
#include <linux/virtio_config.h>
#include <linux/virtio_ring.h>
#include <linux/atomic.h>
+#include <linux/pagemap.h>
+#include <linux/mmu_notifier.h>
struct vhost_work;
typedef void (*vhost_work_fn_t)(struct vhost_work *work);
@@ -80,6 +82,13 @@ enum vhost_uaddr_type {
VHOST_NUM_ADDRS = 3,
};
+struct vhost_vmap {
+ void *addr;
+ void *unmap_addr;
+ int npages;
+ struct page **pages;
+};
+
/* The virtqueue structure describes a queue attached to a device. */
struct vhost_virtqueue {
struct vhost_dev *dev;
@@ -90,6 +99,11 @@ struct vhost_virtqueue {
struct vring_desc __user *desc;
struct vring_avail __user *avail;
struct vring_used __user *used;
+
+ struct vhost_vmap avail_ring;
+ struct vhost_vmap desc_ring;
+ struct vhost_vmap used_ring;
+
const struct vhost_umem_node *meta_iotlb[VHOST_NUM_ADDRS];
struct file *kick;
struct eventfd_ctx *call_ctx;
@@ -158,6 +172,7 @@ struct vhost_msg_node {
struct vhost_dev {
struct mm_struct *mm;
+ struct mmu_notifier mmu_notifier;
struct mutex mutex;
struct vhost_virtqueue **vqs;
int nvqs;
@@ -210,6 +225,7 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len,
struct iovec *iov, int count);
int vq_meta_prefetch(struct vhost_virtqueue *vq);
+void vq_meta_prefetch_done(struct vhost_virtqueue *vq);
struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
void vhost_enqueue_msg(struct vhost_dev *dev,
--
1.8.3.1
^ permalink raw reply related
* [RFC PATCH V2 4/5] vhost: introduce helpers to get the size of metadata area
From: Jason Wang @ 2019-03-06 7:18 UTC (permalink / raw)
To: jasowang, mst, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, linux-mm
In-Reply-To: <1551856692-3384-1-git-send-email-jasowang@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 46 ++++++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2025543..1015464 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -413,6 +413,27 @@ static void vhost_dev_free_iovecs(struct vhost_dev *dev)
vhost_vq_free_iovecs(dev->vqs[i]);
}
+static size_t vhost_get_avail_size(struct vhost_virtqueue *vq, int num)
+{
+ size_t event = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
+
+ return sizeof(*vq->avail) +
+ sizeof(*vq->avail->ring) * num + event;
+}
+
+static size_t vhost_get_used_size(struct vhost_virtqueue *vq, int num)
+{
+ size_t event = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
+
+ return sizeof(*vq->used) +
+ sizeof(*vq->used->ring) * num + event;
+}
+
+static size_t vhost_get_desc_size(struct vhost_virtqueue *vq, int num)
+{
+ return sizeof(*vq->desc) * num;
+}
+
void vhost_dev_init(struct vhost_dev *dev,
struct vhost_virtqueue **vqs, int nvqs, int iov_limit)
{
@@ -1253,13 +1274,9 @@ static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
struct vring_used __user *used)
{
- size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
-
- return access_ok(desc, num * sizeof *desc) &&
- access_ok(avail,
- sizeof *avail + num * sizeof *avail->ring + s) &&
- access_ok(used,
- sizeof *used + num * sizeof *used->ring + s);
+ return access_ok(desc, vhost_get_desc_size(vq, num)) &&
+ access_ok(avail, vhost_get_avail_size(vq, num)) &&
+ access_ok(used, vhost_get_used_size(vq, num));
}
static void vhost_vq_meta_update(struct vhost_virtqueue *vq,
@@ -1311,22 +1328,18 @@ static bool iotlb_access_ok(struct vhost_virtqueue *vq,
int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
- size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
unsigned int num = vq->num;
if (!vq->iotlb)
return 1;
return iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->desc,
- num * sizeof(*vq->desc), VHOST_ADDR_DESC) &&
+ vhost_get_desc_size(vq, num), VHOST_ADDR_DESC) &&
iotlb_access_ok(vq, VHOST_ACCESS_RO, (u64)(uintptr_t)vq->avail,
- sizeof *vq->avail +
- num * sizeof(*vq->avail->ring) + s,
+ vhost_get_avail_size(vq, num),
VHOST_ADDR_AVAIL) &&
iotlb_access_ok(vq, VHOST_ACCESS_WO, (u64)(uintptr_t)vq->used,
- sizeof *vq->used +
- num * sizeof(*vq->used->ring) + s,
- VHOST_ADDR_USED);
+ vhost_get_used_size(vq, num), VHOST_ADDR_USED);
}
EXPORT_SYMBOL_GPL(vq_meta_prefetch);
@@ -1343,13 +1356,10 @@ bool vhost_log_access_ok(struct vhost_dev *dev)
static bool vq_log_access_ok(struct vhost_virtqueue *vq,
void __user *log_base)
{
- size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
-
return vq_memory_access_ok(log_base, vq->umem,
vhost_has_feature(vq, VHOST_F_LOG_ALL)) &&
(!vq->log_used || log_access_ok(log_base, vq->log_addr,
- sizeof *vq->used +
- vq->num * sizeof *vq->used->ring + s));
+ vhost_get_used_size(vq, vq->num)));
}
/* Can we start vq? */
--
1.8.3.1
^ permalink raw reply related
* [RFC PATCH V2 3/5] vhost: rename vq_iotlb_prefetch() to vq_meta_prefetch()
From: Jason Wang @ 2019-03-06 7:18 UTC (permalink / raw)
To: jasowang, mst, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, linux-mm
In-Reply-To: <1551856692-3384-1-git-send-email-jasowang@redhat.com>
Rename the function to be more accurate since it actually tries to
prefetch vq metadata address in IOTLB. And this will be used by
following patch to prefetch metadata virtual addresses.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 4 ++--
drivers/vhost/vhost.c | 4 ++--
drivers/vhost/vhost.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index df51a35..bf55f99 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -971,7 +971,7 @@ static void handle_tx(struct vhost_net *net)
if (!sock)
goto out;
- if (!vq_iotlb_prefetch(vq))
+ if (!vq_meta_prefetch(vq))
goto out;
vhost_disable_notify(&net->dev, vq);
@@ -1140,7 +1140,7 @@ static void handle_rx(struct vhost_net *net)
if (!sock)
goto out;
- if (!vq_iotlb_prefetch(vq))
+ if (!vq_meta_prefetch(vq))
goto out;
vhost_disable_notify(&net->dev, vq);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 29709e7..2025543 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1309,7 +1309,7 @@ static bool iotlb_access_ok(struct vhost_virtqueue *vq,
return true;
}
-int vq_iotlb_prefetch(struct vhost_virtqueue *vq)
+int vq_meta_prefetch(struct vhost_virtqueue *vq)
{
size_t s = vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
unsigned int num = vq->num;
@@ -1328,7 +1328,7 @@ int vq_iotlb_prefetch(struct vhost_virtqueue *vq)
num * sizeof(*vq->used->ring) + s,
VHOST_ADDR_USED);
}
-EXPORT_SYMBOL_GPL(vq_iotlb_prefetch);
+EXPORT_SYMBOL_GPL(vq_meta_prefetch);
/* Can we log writes? */
/* Caller should have device mutex but not vq mutex */
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 9490e7d..7a7fc00 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -209,7 +209,7 @@ void vhost_add_used_and_signal_n(struct vhost_dev *, struct vhost_virtqueue *,
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len,
struct iovec *iov, int count);
-int vq_iotlb_prefetch(struct vhost_virtqueue *vq);
+int vq_meta_prefetch(struct vhost_virtqueue *vq);
struct vhost_msg_node *vhost_new_msg(struct vhost_virtqueue *vq, int type);
void vhost_enqueue_msg(struct vhost_dev *dev,
--
1.8.3.1
^ permalink raw reply related
* [RFC PATCH V2 2/5] vhost: fine grain userspace memory accessors
From: Jason Wang @ 2019-03-06 7:18 UTC (permalink / raw)
To: jasowang, mst, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, linux-mm
In-Reply-To: <1551856692-3384-1-git-send-email-jasowang@redhat.com>
This is used to hide the metadata address from virtqueue helpers. This
will allow to implement a vmap based fast accessing to metadata.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 94 +++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 77 insertions(+), 17 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 400aa78..29709e7 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -869,6 +869,34 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
ret; \
})
+static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
+ vhost_avail_event(vq));
+}
+
+static inline int vhost_put_used(struct vhost_virtqueue *vq,
+ struct vring_used_elem *head, int idx,
+ int count)
+{
+ return vhost_copy_to_user(vq, vq->used->ring + idx, head,
+ count * sizeof(*head));
+}
+
+static inline int vhost_put_used_flags(struct vhost_virtqueue *vq)
+
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
+ &vq->used->flags);
+}
+
+static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
+
+{
+ return vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
+ &vq->used->idx);
+}
+
#define vhost_get_user(vq, x, ptr, type) \
({ \
int ret; \
@@ -907,6 +935,43 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d)
mutex_unlock(&d->vqs[i]->mutex);
}
+static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,
+ __virtio16 *idx)
+{
+ return vhost_get_avail(vq, *idx, &vq->avail->idx);
+}
+
+static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
+ __virtio16 *head, int idx)
+{
+ return vhost_get_avail(vq, *head,
+ &vq->avail->ring[idx & (vq->num - 1)]);
+}
+
+static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
+ __virtio16 *flags)
+{
+ return vhost_get_avail(vq, *flags, &vq->avail->flags);
+}
+
+static inline int vhost_get_used_event(struct vhost_virtqueue *vq,
+ __virtio16 *event)
+{
+ return vhost_get_avail(vq, *event, vhost_used_event(vq));
+}
+
+static inline int vhost_get_used_idx(struct vhost_virtqueue *vq,
+ __virtio16 *idx)
+{
+ return vhost_get_used(vq, *idx, &vq->used->idx);
+}
+
+static inline int vhost_get_desc(struct vhost_virtqueue *vq,
+ struct vring_desc *desc, int idx)
+{
+ return vhost_copy_from_user(vq, desc, vq->desc + idx, sizeof(*desc));
+}
+
static int vhost_new_umem_range(struct vhost_umem *umem,
u64 start, u64 size, u64 end,
u64 userspace_addr, int perm)
@@ -1840,8 +1905,7 @@ int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
static int vhost_update_used_flags(struct vhost_virtqueue *vq)
{
void __user *used;
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->used_flags),
- &vq->used->flags) < 0)
+ if (vhost_put_used_flags(vq))
return -EFAULT;
if (unlikely(vq->log_used)) {
/* Make sure the flag is seen before log. */
@@ -1858,8 +1922,7 @@ static int vhost_update_used_flags(struct vhost_virtqueue *vq)
static int vhost_update_avail_event(struct vhost_virtqueue *vq, u16 avail_event)
{
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->avail_idx),
- vhost_avail_event(vq)))
+ if (vhost_put_avail_event(vq))
return -EFAULT;
if (unlikely(vq->log_used)) {
void __user *used;
@@ -1895,7 +1958,7 @@ int vhost_vq_init_access(struct vhost_virtqueue *vq)
r = -EFAULT;
goto err;
}
- r = vhost_get_used(vq, last_used_idx, &vq->used->idx);
+ r = vhost_get_used_idx(vq, &last_used_idx);
if (r) {
vq_err(vq, "Can't access used idx at %p\n",
&vq->used->idx);
@@ -2094,7 +2157,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
last_avail_idx = vq->last_avail_idx;
if (vq->avail_idx == vq->last_avail_idx) {
- if (unlikely(vhost_get_avail(vq, avail_idx, &vq->avail->idx))) {
+ if (unlikely(vhost_get_avail_idx(vq, &avail_idx))) {
vq_err(vq, "Failed to access avail idx at %p\n",
&vq->avail->idx);
return -EFAULT;
@@ -2121,8 +2184,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
- if (unlikely(vhost_get_avail(vq, ring_head,
- &vq->avail->ring[last_avail_idx & (vq->num - 1)]))) {
+ if (unlikely(vhost_get_avail_head(vq, &ring_head, last_avail_idx))) {
vq_err(vq, "Failed to read head: idx %d address %p\n",
last_avail_idx,
&vq->avail->ring[last_avail_idx % vq->num]);
@@ -2157,8 +2219,7 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
i, vq->num, head);
return -EINVAL;
}
- ret = vhost_copy_from_user(vq, &desc, vq->desc + i,
- sizeof desc);
+ ret = vhost_get_desc(vq, &desc, i);
if (unlikely(ret)) {
vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
i, vq->desc + i);
@@ -2251,7 +2312,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
start = vq->last_used_idx & (vq->num - 1);
used = vq->used->ring + start;
- if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
+ if (vhost_put_used(vq, heads, start, count)) {
vq_err(vq, "Failed to write used");
return -EFAULT;
}
@@ -2293,8 +2354,7 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
/* Make sure buffer is written before we update index. */
smp_wmb();
- if (vhost_put_user(vq, cpu_to_vhost16(vq, vq->last_used_idx),
- &vq->used->idx)) {
+ if (vhost_put_used_idx(vq)) {
vq_err(vq, "Failed to increment used idx");
return -EFAULT;
}
@@ -2327,7 +2387,7 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (!vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {
__virtio16 flags;
- if (vhost_get_avail(vq, flags, &vq->avail->flags)) {
+ if (vhost_get_avail_flags(vq, &flags)) {
vq_err(vq, "Failed to get flags");
return true;
}
@@ -2341,7 +2401,7 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (unlikely(!v))
return true;
- if (vhost_get_avail(vq, event, vhost_used_event(vq))) {
+ if (vhost_get_used_event(vq, &event)) {
vq_err(vq, "Failed to get used event idx");
return true;
}
@@ -2386,7 +2446,7 @@ bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (vq->avail_idx != vq->last_avail_idx)
return false;
- r = vhost_get_avail(vq, avail_idx, &vq->avail->idx);
+ r = vhost_get_avail_idx(vq, &avail_idx);
if (unlikely(r))
return false;
vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
@@ -2422,7 +2482,7 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
/* They could have slipped one in as we were doing that: make
* sure it's written, then check again. */
smp_mb();
- r = vhost_get_avail(vq, avail_idx, &vq->avail->idx);
+ r = vhost_get_avail_idx(vq, &avail_idx);
if (r) {
vq_err(vq, "Failed to check avail idx at %p: %d\n",
&vq->avail->idx, r);
--
1.8.3.1
^ permalink raw reply related
* [RFC PATCH V2 1/5] vhost: generalize adding used elem
From: Jason Wang @ 2019-03-06 7:18 UTC (permalink / raw)
To: jasowang, mst, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, linux-mm
In-Reply-To: <1551856692-3384-1-git-send-email-jasowang@redhat.com>
Use one generic vhost_copy_to_user() instead of two dedicated
accessor. This will simplify the conversion to fine grain
accessors. About 2% improvement of PPS were seen during vitio-user
txonly test.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index a2e5dc7..400aa78 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2251,16 +2251,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
start = vq->last_used_idx & (vq->num - 1);
used = vq->used->ring + start;
- if (count == 1) {
- if (vhost_put_user(vq, heads[0].id, &used->id)) {
- vq_err(vq, "Failed to write used id");
- return -EFAULT;
- }
- if (vhost_put_user(vq, heads[0].len, &used->len)) {
- vq_err(vq, "Failed to write used len");
- return -EFAULT;
- }
- } else if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
+ if (vhost_copy_to_user(vq, used, heads, count * sizeof *used)) {
vq_err(vq, "Failed to write used");
return -EFAULT;
}
--
1.8.3.1
^ permalink raw reply related
* [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
From: Jason Wang @ 2019-03-06 7:18 UTC (permalink / raw)
To: jasowang, mst, kvm, virtualization, netdev, linux-kernel
Cc: aarcange, linux-mm
This series tries to access virtqueue metadata through kernel virtual
address instead of copy_user() friends since they had too much
overheads like checks, spec barriers or even hardware feature
toggling. This is done through setup kernel address through vmap() and
resigter MMU notifier for invalidation.
Test shows about 24% improvement on TX PPS. TCP_STREAM doesn't see
obvious improvement.
Thanks
Changes from V4:
- use invalidate_range() instead of invalidate_range_start()
- track dirty pages
Changes from V3:
- don't try to use vmap for file backed pages
- rebase to master
Changes from V2:
- fix buggy range overlapping check
- tear down MMU notifier during vhost ioctl to make sure invalidation
request can read metadata userspace address and vq size without
holding vq mutex.
Changes from V1:
- instead of pinning pages, use MMU notifier to invalidate vmaps and
remap duing metadata prefetch
- fix build warning on MIPS
Jason Wang (5):
vhost: generalize adding used elem
vhost: fine grain userspace memory accessors
vhost: rename vq_iotlb_prefetch() to vq_meta_prefetch()
vhost: introduce helpers to get the size of metadata area
vhost: access vq metadata through kernel virtual address
drivers/vhost/net.c | 6 +-
drivers/vhost/vhost.c | 434 ++++++++++++++++++++++++++++++++++++++++++++------
drivers/vhost/vhost.h | 18 ++-
3 files changed, 407 insertions(+), 51 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-06 6:43 UTC (permalink / raw)
To: si-wei liu
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, virtualization,
liran.alon, Netdev, boris.ostrovsky, David Miller
In-Reply-To: <fde572b3-e342-b37f-0e3d-e16599f1ee81@oracle.com>
On Tue, Mar 05, 2019 at 04:51:00PM -0800, si-wei liu wrote:
>
>
> On 3/5/2019 4:36 PM, Michael S. Tsirkin wrote:
> > On Tue, Mar 05, 2019 at 04:20:50PM -0800, si-wei liu wrote:
> > >
> > > On 3/5/2019 4:06 PM, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 05, 2019 at 11:35:50AM -0800, si-wei liu wrote:
> > > > > On 3/5/2019 11:24 AM, Stephen Hemminger wrote:
> > > > > > On Tue, 5 Mar 2019 11:19:32 -0800
> > > > > > si-wei liu <si-wei.liu@oracle.com> wrote:
> > > > > >
> > > > > > > > I have a vague idea: would it work to *not* set
> > > > > > > > IFF_UP on slave devices at all?
> > > > > > > Hmm, I ever thought about this option, and it appears this solution is
> > > > > > > more invasive than required to convert existing scripts, despite the
> > > > > > > controversy of introducing internal netdev state to differentiate user
> > > > > > > visible state. Either we disallow slave to be brought up by user, or to
> > > > > > > not set IFF_UP flag but instead use the internal one, could end up with
> > > > > > > substantial behavioral change that breaks scripts. Consider any admin
> > > > > > > script that does `ip link set dev ... up' successfully just assumes the
> > > > > > > link is up and subsequent operation can be done as usual.
> > > > How would it work when carrier is off?
> > > >
> > > > > While it *may*
> > > > > > > work for dracut (yet to be verified), I'm a bit concerned that there are
> > > > > > > more scripts to be converted than those that don't follow volatile
> > > > > > > failover slave names. It's technically doable, but may not worth the
> > > > > > > effort (in terms of porting existing scripts/apps).
> > > > > > >
> > > > > > > Thanks
> > > > > > > -Siwei
> > > > > > Won't work for most devices. Many devices turn off PHY and link layer
> > > > > > if not IFF_UP
> > > > > True, that's what I said about introducing internal state for those driver
> > > > > and other kernel component. Very invasive change indeed.
> > > > >
> > > > > -Siwei
> > > > Well I did say it's vague.
> > > > How about hiding IFF_UP from dev_get_flags (and probably
> > > > __dev_change_flags)?
> > > >
> > > Any different? This has small footprint for the kernel change for sure,
> > > while the discrepancy is still there. Anyone who writes code for IFF_UP will
> > > not notice IFF_FAILOVER_SLAVE.
> > >
> > > Not to mention more userspace "fixup" work has to be done due to this
> > > change.
> > >
> > > -Siwei
> > >
> > >
> > Point is it's ok since most userspace should just ignore slaves
> > - hopefully it will just ignore it since it already
> > ignores interfaces that are down.
> Admin script thought the interface could be bright up and do further
> operations without checking the UP flag.
These scripts then would be broken on any box with multiple interfaces
since not all of these would have carrier.
> It doesn't look to be a reliable
> way of prohibit userspace from operating against slaves.
>
> -Siwei
>
>
This does not mean we shouldn't make an effort to disable broken
configurations.
I am not arguing against your patch. Not at all. I see better
hiding of slaves as a separate enhancement.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
--
MST
^ permalink raw reply
* Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-06 0:36 UTC (permalink / raw)
To: si-wei liu
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, virtualization,
liran.alon, Netdev, boris.ostrovsky, David Miller
In-Reply-To: <8737e985-f418-7002-c8b5-0023d1c4a453@oracle.com>
On Tue, Mar 05, 2019 at 04:20:50PM -0800, si-wei liu wrote:
>
>
> On 3/5/2019 4:06 PM, Michael S. Tsirkin wrote:
> > On Tue, Mar 05, 2019 at 11:35:50AM -0800, si-wei liu wrote:
> > >
> > > On 3/5/2019 11:24 AM, Stephen Hemminger wrote:
> > > > On Tue, 5 Mar 2019 11:19:32 -0800
> > > > si-wei liu <si-wei.liu@oracle.com> wrote:
> > > >
> > > > > > I have a vague idea: would it work to *not* set
> > > > > > IFF_UP on slave devices at all?
> > > > > Hmm, I ever thought about this option, and it appears this solution is
> > > > > more invasive than required to convert existing scripts, despite the
> > > > > controversy of introducing internal netdev state to differentiate user
> > > > > visible state. Either we disallow slave to be brought up by user, or to
> > > > > not set IFF_UP flag but instead use the internal one, could end up with
> > > > > substantial behavioral change that breaks scripts. Consider any admin
> > > > > script that does `ip link set dev ... up' successfully just assumes the
> > > > > link is up and subsequent operation can be done as usual.
> > How would it work when carrier is off?
> >
> > > While it *may*
> > > > > work for dracut (yet to be verified), I'm a bit concerned that there are
> > > > > more scripts to be converted than those that don't follow volatile
> > > > > failover slave names. It's technically doable, but may not worth the
> > > > > effort (in terms of porting existing scripts/apps).
> > > > >
> > > > > Thanks
> > > > > -Siwei
> > > > Won't work for most devices. Many devices turn off PHY and link layer
> > > > if not IFF_UP
> > > True, that's what I said about introducing internal state for those driver
> > > and other kernel component. Very invasive change indeed.
> > >
> > > -Siwei
> > Well I did say it's vague.
> > How about hiding IFF_UP from dev_get_flags (and probably
> > __dev_change_flags)?
> >
> Any different? This has small footprint for the kernel change for sure,
> while the discrepancy is still there. Anyone who writes code for IFF_UP will
> not notice IFF_FAILOVER_SLAVE.
>
> Not to mention more userspace "fixup" work has to be done due to this
> change.
>
> -Siwei
>
>
Point is it's ok since most userspace should just ignore slaves
- hopefully it will just ignore it since it already
ignores interfaces that are down.
--
MST
^ permalink raw reply
* Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-06 0:06 UTC (permalink / raw)
To: si-wei liu
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, virtualization,
liran.alon, Netdev, boris.ostrovsky, David Miller
In-Reply-To: <9448ae8f-c4e0-58a4-ff46-2f2951113d1e@oracle.com>
On Tue, Mar 05, 2019 at 11:35:50AM -0800, si-wei liu wrote:
>
>
> On 3/5/2019 11:24 AM, Stephen Hemminger wrote:
> > On Tue, 5 Mar 2019 11:19:32 -0800
> > si-wei liu <si-wei.liu@oracle.com> wrote:
> >
> > > > I have a vague idea: would it work to *not* set
> > > > IFF_UP on slave devices at all?
> > > Hmm, I ever thought about this option, and it appears this solution is
> > > more invasive than required to convert existing scripts, despite the
> > > controversy of introducing internal netdev state to differentiate user
> > > visible state. Either we disallow slave to be brought up by user, or to
> > > not set IFF_UP flag but instead use the internal one, could end up with
> > > substantial behavioral change that breaks scripts. Consider any admin
> > > script that does `ip link set dev ... up' successfully just assumes the
> > > link is up and subsequent operation can be done as usual.
How would it work when carrier is off?
> While it *may*
> > > work for dracut (yet to be verified), I'm a bit concerned that there are
> > > more scripts to be converted than those that don't follow volatile
> > > failover slave names. It's technically doable, but may not worth the
> > > effort (in terms of porting existing scripts/apps).
> > >
> > > Thanks
> > > -Siwei
> > Won't work for most devices. Many devices turn off PHY and link layer
> > if not IFF_UP
> True, that's what I said about introducing internal state for those driver
> and other kernel component. Very invasive change indeed.
>
> -Siwei
Well I did say it's vague.
How about hiding IFF_UP from dev_get_flags (and probably
__dev_change_flags)?
^ permalink raw reply
* Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-05 20:28 UTC (permalink / raw)
To: si-wei liu
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, virtualization,
liran.alon, Netdev, boris.ostrovsky, David Miller
In-Reply-To: <e3b34115-39ca-ac5f-7d53-8bf9e69c910d@oracle.com>
On Tue, Mar 05, 2019 at 11:19:32AM -0800, si-wei liu wrote:
>
>
> On 3/4/2019 6:33 PM, Michael S. Tsirkin wrote:
> > On Mon, Mar 04, 2019 at 07:50:59PM -0500, 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.
> > >
> > > For that to work, now introduce a module-level tunable,
> > > "slave_rename_ok" that allows users to lift up the rename restriction on
> > > failover slave which is already UP. Although it's possible this change
> > > 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 work with the new naming
> > > behavior of the failover slave. Userspace component interacting with
> > > slaves should be changed to operate on failover master 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
> > > all userspace should only deal with master in the long run. The default
> > > for the "slave_rename_ok" is set to true(1). If userspace doesn't have
> > > the right support in place meanwhile users don't care about reliable
> > > userspace naming, the value can be set to false(0).
> > >
> > > Signed-off-by: Si-Wei.Liu@oracle.com
> > > Reviewed-by: Liran Alon <liran.alon@oracle.com>
> > Not sure which of the versions I should reply to.
> Sorry for multiple copies sent. It's fine to reply to this one.
>
> >
> > I have a vague idea: would it work to *not* set
> > IFF_UP on slave devices at all?
> Hmm, I ever thought about this option, and it appears this solution is more
> invasive than required to convert existing scripts, despite the controversy
> of introducing internal netdev state to differentiate user visible state.
> Either we disallow slave to be brought up by user, or to not set IFF_UP flag
> but instead use the internal one, could end up with substantial behavioral
> change that breaks scripts. Consider any admin script that does `ip link set
> dev ... up' successfully just assumes the link is up and subsequent
> operation can be done as usual. While it *may* work for dracut (yet to be
> verified), I'm a bit concerned that there are more scripts to be converted
> than those that don't follow volatile failover slave names. It's technically
> doable, but may not worth the effort (in terms of porting existing
> scripts/apps).
>
> Thanks
> -Siwei
Right. Advantage could be that we prevent all kind of
misconfigurations e.g. when one has a route on a slave.
> >
> > Would this reduce the chances of existing scripts such as dracut being
> > confused?
> >
> > And this leaves open the option for scripts to address
> > slaves by checking some custom attribute.
> >
> > > ---
> > > include/linux/netdevice.h | 3 +++
> > > net/core/dev.c | 3 ++-
> > > net/core/failover.c | 11 +++++++++--
> > > 3 files changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index 857f8ab..6d9e4e0 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -1487,6 +1487,7 @@ struct net_device_ops {
> > > * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> > > * @IFF_FAILOVER: device is a failover master device
> > > * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> > > + * @IFF_SLAVE_RENAME_OK: rename is allowed while slave device is running
> > > */
> > > enum netdev_priv_flags {
> > > IFF_802_1Q_VLAN = 1<<0,
> > > @@ -1518,6 +1519,7 @@ enum netdev_priv_flags {
> > > IFF_NO_RX_HANDLER = 1<<26,
> > > IFF_FAILOVER = 1<<27,
> > > IFF_FAILOVER_SLAVE = 1<<28,
> > > + IFF_SLAVE_RENAME_OK = 1<<29,
> > > };
> > > #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
> > > @@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
> > > #define IFF_NO_RX_HANDLER IFF_NO_RX_HANDLER
> > > #define IFF_FAILOVER IFF_FAILOVER
> > > #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
> > > +#define IFF_SLAVE_RENAME_OK IFF_SLAVE_RENAME_OK
> > > /**
> > > * struct net_device - The DEVICE structure.
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 722d50d..ae070de 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -1180,7 +1180,8 @@ 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)
> > > + if (dev->flags & IFF_UP &&
> > > + !(dev->priv_flags & IFF_SLAVE_RENAME_OK))
> > > return -EBUSY;
> > > write_seqcount_begin(&devnet_rename_seq);
> > > diff --git a/net/core/failover.c b/net/core/failover.c
> > > index 4a92a98..1fd8bbb 100644
> > > --- a/net/core/failover.c
> > > +++ b/net/core/failover.c
> > > @@ -16,6 +16,11 @@
> > > static LIST_HEAD(failover_list);
> > > static DEFINE_SPINLOCK(failover_lock);
> > > +static bool slave_rename_ok = true;
> > > +
> > > +module_param(slave_rename_ok, bool, (S_IRUGO | S_IWUSR));
> > > +MODULE_PARM_DESC(slave_rename_ok,
> > > + "If set allow renaming the slave when failover master is up");
> > > static struct net_device *failover_get_bymac(u8 *mac, struct failover_ops **ops)
> > > {
> > > @@ -81,13 +86,15 @@ static int failover_slave_register(struct net_device *slave_dev)
> > > }
> > > slave_dev->priv_flags |= IFF_FAILOVER_SLAVE;
> > > + if (slave_rename_ok)
> > > + slave_dev->priv_flags |= IFF_SLAVE_RENAME_OK;
> > > 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_SLAVE_RENAME_OK);
> > > err_upper_link:
> > > netdev_rx_handler_unregister(slave_dev);
> > > done:
> > > @@ -121,7 +128,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_SLAVE_RENAME_OK);
> > > if (fops && fops->slave_unregister &&
> > > !fops->slave_unregister(slave_dev, failover_dev))
> > > --
> > > 1.8.3.1
^ permalink raw reply
* Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces
From: Stephen Hemminger @ 2019-03-05 19:24 UTC (permalink / raw)
To: si-wei liu
Cc: Jiri Pirko, Michael S. Tsirkin, Jakub Kicinski, Sridhar Samudrala,
virtualization, liran.alon, Netdev, boris.ostrovsky, David Miller
In-Reply-To: <e3b34115-39ca-ac5f-7d53-8bf9e69c910d@oracle.com>
On Tue, 5 Mar 2019 11:19:32 -0800
si-wei liu <si-wei.liu@oracle.com> wrote:
> > I have a vague idea: would it work to *not* set
> > IFF_UP on slave devices at all?
> Hmm, I ever thought about this option, and it appears this solution is
> more invasive than required to convert existing scripts, despite the
> controversy of introducing internal netdev state to differentiate user
> visible state. Either we disallow slave to be brought up by user, or to
> not set IFF_UP flag but instead use the internal one, could end up with
> substantial behavioral change that breaks scripts. Consider any admin
> script that does `ip link set dev ... up' successfully just assumes the
> link is up and subsequent operation can be done as usual. While it *may*
> work for dracut (yet to be verified), I'm a bit concerned that there are
> more scripts to be converted than those that don't follow volatile
> failover slave names. It's technically doable, but may not worth the
> effort (in terms of porting existing scripts/apps).
>
> Thanks
> -Siwei
Won't work for most devices. Many devices turn off PHY and link layer
if not IFF_UP
^ permalink raw reply
* [PATCH] vsock/virtio: fix kernel panic from virtio_transport_reset_no_sock
From: Adalbert Lazăr @ 2019-03-05 18:01 UTC (permalink / raw)
To: Stefan Hajnoczi, David S . Miller, Stefano Garzarella
Cc: Adalbert Lazăr, netdev, linux-kernel, kvm, virtualization
Previous to commit 22b5c0b63f32 ("vsock/virtio: fix kernel panic after device hot-unplug"),
vsock_core_init() was called from virtio_vsock_probe(). Now,
virtio_transport_reset_no_sock() can be called before vsock_core_init()
has the chance to run.
[Wed Feb 27 14:17:09 2019] BUG: unable to handle kernel NULL pointer dereference at 0000000000000110
[Wed Feb 27 14:17:09 2019] #PF error: [normal kernel read fault]
[Wed Feb 27 14:17:09 2019] PGD 0 P4D 0
[Wed Feb 27 14:17:09 2019] Oops: 0000 [#1] SMP PTI
[Wed Feb 27 14:17:09 2019] CPU: 3 PID: 59 Comm: kworker/3:1 Not tainted 5.0.0-rc7-390-generic-hvi #390
[Wed Feb 27 14:17:09 2019] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
[Wed Feb 27 14:17:09 2019] Workqueue: virtio_vsock virtio_transport_rx_work [vmw_vsock_virtio_transport]
[Wed Feb 27 14:17:09 2019] RIP: 0010:virtio_transport_reset_no_sock+0x8c/0xc0 [vmw_vsock_virtio_transport_common]
[Wed Feb 27 14:17:09 2019] Code: 35 8b 4f 14 48 8b 57 08 31 f6 44 8b 4f 10 44 8b 07 48 8d 7d c8 e8 84 f8 ff ff 48 85 c0 48 89 c3 74 2a e8 f7 31 03 00 48 89 df <48> 8b 80 10 01 00 00 e8 68 fb 69 ed 48 8b 75 f0 65 48 33 34 25 28
[Wed Feb 27 14:17:09 2019] RSP: 0018:ffffb42701ab7d40 EFLAGS: 00010282
[Wed Feb 27 14:17:09 2019] RAX: 0000000000000000 RBX: ffff9d79637ee080 RCX: 0000000000000003
[Wed Feb 27 14:17:09 2019] RDX: 0000000000000001 RSI: 0000000000000002 RDI: ffff9d79637ee080
[Wed Feb 27 14:17:09 2019] RBP: ffffb42701ab7d78 R08: ffff9d796fae70e0 R09: ffff9d796f403500
[Wed Feb 27 14:17:09 2019] R10: ffffb42701ab7d90 R11: 0000000000000000 R12: ffff9d7969d09240
[Wed Feb 27 14:17:09 2019] R13: ffff9d79624e6840 R14: ffff9d7969d09318 R15: ffff9d796d48ff80
[Wed Feb 27 14:17:09 2019] FS: 0000000000000000(0000) GS:ffff9d796fac0000(0000) knlGS:0000000000000000
[Wed Feb 27 14:17:09 2019] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[Wed Feb 27 14:17:09 2019] CR2: 0000000000000110 CR3: 0000000427f22000 CR4: 00000000000006e0
[Wed Feb 27 14:17:09 2019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[Wed Feb 27 14:17:09 2019] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[Wed Feb 27 14:17:09 2019] Call Trace:
[Wed Feb 27 14:17:09 2019] virtio_transport_recv_pkt+0x63/0x820 [vmw_vsock_virtio_transport_common]
[Wed Feb 27 14:17:09 2019] ? kfree+0x17e/0x190
[Wed Feb 27 14:17:09 2019] ? detach_buf_split+0x145/0x160
[Wed Feb 27 14:17:09 2019] ? __switch_to_asm+0x40/0x70
[Wed Feb 27 14:17:09 2019] virtio_transport_rx_work+0xa0/0x106 [vmw_vsock_virtio_transport]
[Wed Feb 27 14:17:09 2019] NET: Registered protocol family 40
[Wed Feb 27 14:17:09 2019] process_one_work+0x167/0x410
[Wed Feb 27 14:17:09 2019] worker_thread+0x4d/0x460
[Wed Feb 27 14:17:09 2019] kthread+0x105/0x140
[Wed Feb 27 14:17:09 2019] ? rescuer_thread+0x360/0x360
[Wed Feb 27 14:17:09 2019] ? kthread_destroy_worker+0x50/0x50
[Wed Feb 27 14:17:09 2019] ret_from_fork+0x35/0x40
[Wed Feb 27 14:17:09 2019] Modules linked in: vmw_vsock_virtio_transport vmw_vsock_virtio_transport_common input_leds vsock serio_raw i2c_piix4 mac_hid qemu_fw_cfg autofs4 cirrus ttm drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops virtio_net psmouse drm net_failover pata_acpi virtio_blk failover floppy
[Wed Feb 27 14:17:09 2019] CR2: 0000000000000110
[Wed Feb 27 14:17:09 2019] ---[ end trace baa35abd2e040fe5 ]---
[Wed Feb 27 14:17:09 2019] RIP: 0010:virtio_transport_reset_no_sock+0x8c/0xc0 [vmw_vsock_virtio_transport_common]
[Wed Feb 27 14:17:09 2019] Code: 35 8b 4f 14 48 8b 57 08 31 f6 44 8b 4f 10 44 8b 07 48 8d 7d c8 e8 84 f8 ff ff 48 85 c0 48 89 c3 74 2a e8 f7 31 03 00 48 89 df <48> 8b 80 10 01 00 00 e8 68 fb 69 ed 48 8b 75 f0 65 48 33 34 25 28
[Wed Feb 27 14:17:09 2019] RSP: 0018:ffffb42701ab7d40 EFLAGS: 00010282
[Wed Feb 27 14:17:09 2019] RAX: 0000000000000000 RBX: ffff9d79637ee080 RCX: 0000000000000003
[Wed Feb 27 14:17:09 2019] RDX: 0000000000000001 RSI: 0000000000000002 RDI: ffff9d79637ee080
[Wed Feb 27 14:17:09 2019] RBP: ffffb42701ab7d78 R08: ffff9d796fae70e0 R09: ffff9d796f403500
[Wed Feb 27 14:17:09 2019] R10: ffffb42701ab7d90 R11: 0000000000000000 R12: ffff9d7969d09240
[Wed Feb 27 14:17:09 2019] R13: ffff9d79624e6840 R14: ffff9d7969d09318 R15: ffff9d796d48ff80
[Wed Feb 27 14:17:09 2019] FS: 0000000000000000(0000) GS:ffff9d796fac0000(0000) knlGS:0000000000000000
[Wed Feb 27 14:17:09 2019] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[Wed Feb 27 14:17:09 2019] CR2: 0000000000000110 CR3: 0000000427f22000 CR4: 00000000000006e0
[Wed Feb 27 14:17:09 2019] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[Wed Feb 27 14:17:09 2019] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
---
net/vmw_vsock/virtio_transport_common.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 3ae3a33da70b..502201aaff2a 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -662,6 +662,7 @@ static int virtio_transport_reset(struct vsock_sock *vsk,
*/
static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
{
+ const struct virtio_transport *t;
struct virtio_vsock_pkt_info info = {
.op = VIRTIO_VSOCK_OP_RST,
.type = le16_to_cpu(pkt->hdr.type),
@@ -680,7 +681,11 @@ static int virtio_transport_reset_no_sock(struct virtio_vsock_pkt *pkt)
if (!pkt)
return -ENOMEM;
- return virtio_transport_get_ops()->send_pkt(pkt);
+ t = virtio_transport_get_ops();
+ if (!t)
+ return -ENOTCONN;
+
+ return t->send_pkt(pkt);
}
static void virtio_transport_wait_close(struct sock *sk, long timeout)
--
2.21.0
^ permalink raw reply related
* Re: [PATCH] virtio_console: free unused buffers with virtio port
From: Pankaj Gupta @ 2019-03-05 7:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: arnd, amit, Gal Hammer, gregkh, linux-kernel, virtualization,
xiaohli
In-Reply-To: <20190304142506-mutt-send-email-mst@kernel.org>
Hello Michael,
Thanks for your reply.
>
> On Mon, Mar 04, 2019 at 06:35:11PM +0530, Pankaj Gupta wrote:
> > The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> > deffered detaching of unused buffer to virtio device unplug time.
> >
> > This causes unplug/replug of single port in virtio device with an
> > error "Error allocating inbufs\n". As we don't free the unused buffers
> > attached with the port. Re-plug the same port tries to allocate new
> > buffers in virtqueue and results in this error if queue is full.
> >
> > This patch removes the unused buffers in vq's when we unplug the port.
> > This is the best we can do as we cannot call device_reset because virtio
> > device is still active. This was the working behaviour before the change
> > introduced in commit b3258ff1d6.
> >
> > Reported-by: Xiaohui Li <xiaohli@redhat.com>
> > Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
>
> I think if you do this you need to add support
> in the packed ring.
o.k. I will look at the implementation details for "support
of packed ring" for virtio_console. This will take some time.
Meanwhile "virtio_console" port hotplug/unplug is broken in upstream.
Can we accept this patch as it fixes the upstream and together
with parent patch(b3258ff1d6) does nice cleanups as well.
Thanks,
Pankaj
>
> > ---
> > drivers/char/virtio_console.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index fbeb71953526..5fbf2ac73111 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> > kfree(port);
> > }
> >
> > +static void remove_unused_bufs(struct virtqueue *vq)
> > +{
> > + struct port_buffer *buf;
> > +
> > + while ((buf = virtqueue_detach_unused_buf(vq)))
> > + free_buf(buf, true);
> > +}
> > +
> > static void remove_port_data(struct port *port)
> > {
> > spin_lock_irq(&port->inbuf_lock);
> > /* Remove unused data this port might have received. */
> > discard_port_data(port);
> > + remove_unused_bufs(port->in_vq);
> > spin_unlock_irq(&port->inbuf_lock);
> >
> > spin_lock_irq(&port->outvq_lock);
> > reclaim_consumed_buffers(port);
> > + remove_unused_bufs(port->out_vq);
> > spin_unlock_irq(&port->outvq_lock);
> > }
> >
> > @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> > struct virtqueue *vq;
> >
> > virtio_device_for_each_vq(portdev->vdev, vq) {
> > - struct port_buffer *buf;
> >
> > flush_bufs(vq, true);
> > - while ((buf = virtqueue_detach_unused_buf(vq)))
> > - free_buf(buf, true);
> > + remove_unused_bufs(vq);
> > }
> > portdev->vdev->config->del_vqs(portdev->vdev);
> > kfree(portdev->in_vqs);
> > --
> > 2.20.1
>
^ permalink raw reply
* Re: [RFC PATCH net-next] failover: allow name change on IFF_UP slave interfaces
From: Michael S. Tsirkin @ 2019-03-05 2:33 UTC (permalink / raw)
To: Si-Wei Liu
Cc: Jiri Pirko, Jakub Kicinski, Sridhar Samudrala, virtualization,
liran.alon, Netdev, boris.ostrovsky, David Miller
In-Reply-To: <1551747059-11831-1-git-send-email-si-wei.liu@oracle.com>
On Mon, Mar 04, 2019 at 07:50:59PM -0500, 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.
>
> For that to work, now introduce a module-level tunable,
> "slave_rename_ok" that allows users to lift up the rename restriction on
> failover slave which is already UP. Although it's possible this change
> 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 work with the new naming
> behavior of the failover slave. Userspace component interacting with
> slaves should be changed to operate on failover master 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
> all userspace should only deal with master in the long run. The default
> for the "slave_rename_ok" is set to true(1). If userspace doesn't have
> the right support in place meanwhile users don't care about reliable
> userspace naming, the value can be set to false(0).
>
> Signed-off-by: Si-Wei.Liu@oracle.com
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
Not sure which of the versions I should reply to.
I have a vague idea: would it work to *not* set
IFF_UP on slave devices at all?
Would this reduce the chances of existing scripts such as dracut being
confused?
And this leaves open the option for scripts to address
slaves by checking some custom attribute.
> ---
> include/linux/netdevice.h | 3 +++
> net/core/dev.c | 3 ++-
> net/core/failover.c | 11 +++++++++--
> 3 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 857f8ab..6d9e4e0 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1487,6 +1487,7 @@ struct net_device_ops {
> * @IFF_NO_RX_HANDLER: device doesn't support the rx_handler hook
> * @IFF_FAILOVER: device is a failover master device
> * @IFF_FAILOVER_SLAVE: device is lower dev of a failover master device
> + * @IFF_SLAVE_RENAME_OK: rename is allowed while slave device is running
> */
> enum netdev_priv_flags {
> IFF_802_1Q_VLAN = 1<<0,
> @@ -1518,6 +1519,7 @@ enum netdev_priv_flags {
> IFF_NO_RX_HANDLER = 1<<26,
> IFF_FAILOVER = 1<<27,
> IFF_FAILOVER_SLAVE = 1<<28,
> + IFF_SLAVE_RENAME_OK = 1<<29,
> };
>
> #define IFF_802_1Q_VLAN IFF_802_1Q_VLAN
> @@ -1548,6 +1550,7 @@ enum netdev_priv_flags {
> #define IFF_NO_RX_HANDLER IFF_NO_RX_HANDLER
> #define IFF_FAILOVER IFF_FAILOVER
> #define IFF_FAILOVER_SLAVE IFF_FAILOVER_SLAVE
> +#define IFF_SLAVE_RENAME_OK IFF_SLAVE_RENAME_OK
>
> /**
> * struct net_device - The DEVICE structure.
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 722d50d..ae070de 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1180,7 +1180,8 @@ 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)
> + if (dev->flags & IFF_UP &&
> + !(dev->priv_flags & IFF_SLAVE_RENAME_OK))
> return -EBUSY;
>
> write_seqcount_begin(&devnet_rename_seq);
> diff --git a/net/core/failover.c b/net/core/failover.c
> index 4a92a98..1fd8bbb 100644
> --- a/net/core/failover.c
> +++ b/net/core/failover.c
> @@ -16,6 +16,11 @@
>
> static LIST_HEAD(failover_list);
> static DEFINE_SPINLOCK(failover_lock);
> +static bool slave_rename_ok = true;
> +
> +module_param(slave_rename_ok, bool, (S_IRUGO | S_IWUSR));
> +MODULE_PARM_DESC(slave_rename_ok,
> + "If set allow renaming the slave when failover master is up");
>
> static struct net_device *failover_get_bymac(u8 *mac, struct failover_ops **ops)
> {
> @@ -81,13 +86,15 @@ static int failover_slave_register(struct net_device *slave_dev)
> }
>
> slave_dev->priv_flags |= IFF_FAILOVER_SLAVE;
> + if (slave_rename_ok)
> + slave_dev->priv_flags |= IFF_SLAVE_RENAME_OK;
>
> 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_SLAVE_RENAME_OK);
> err_upper_link:
> netdev_rx_handler_unregister(slave_dev);
> done:
> @@ -121,7 +128,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_SLAVE_RENAME_OK);
>
> if (fops && fops->slave_unregister &&
> !fops->slave_unregister(slave_dev, failover_dev))
> --
> 1.8.3.1
^ permalink raw reply
* Re: [RFC PATCH net] failover: allow name change on IFF_UP slave interfaces
From: David Miller @ 2019-03-05 2:04 UTC (permalink / raw)
To: si-wei.liu
Cc: jiri, mst, kubakici, sridhar.samudrala, virtualization,
liran.alon, netdev, boris.ostrovsky
In-Reply-To: <1551747198-11911-1-git-send-email-si-wei.liu@oracle.com>
Why did you send this three times?
What's different in each of these copies?
^ permalink raw reply
* Re: [PATCH] virtio_console: free unused buffers with virtio port
From: Michael S. Tsirkin @ 2019-03-04 19:25 UTC (permalink / raw)
To: Pankaj Gupta; +Cc: arnd, amit, gregkh, linux-kernel, virtualization, xiaohli
In-Reply-To: <20190304130511.14450-1-pagupta@redhat.com>
On Mon, Mar 04, 2019 at 06:35:11PM +0530, Pankaj Gupta wrote:
> The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> deffered detaching of unused buffer to virtio device unplug time.
>
> This causes unplug/replug of single port in virtio device with an
> error "Error allocating inbufs\n". As we don't free the unused buffers
> attached with the port. Re-plug the same port tries to allocate new
> buffers in virtqueue and results in this error if queue is full.
>
> This patch removes the unused buffers in vq's when we unplug the port.
> This is the best we can do as we cannot call device_reset because virtio
> device is still active. This was the working behaviour before the change
> introduced in commit b3258ff1d6.
>
> Reported-by: Xiaohui Li <xiaohli@redhat.com>
> Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
I think if you do this you need to add support
in the packed ring.
> ---
> drivers/char/virtio_console.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fbeb71953526..5fbf2ac73111 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> kfree(port);
> }
>
> +static void remove_unused_bufs(struct virtqueue *vq)
> +{
> + struct port_buffer *buf;
> +
> + while ((buf = virtqueue_detach_unused_buf(vq)))
> + free_buf(buf, true);
> +}
> +
> static void remove_port_data(struct port *port)
> {
> spin_lock_irq(&port->inbuf_lock);
> /* Remove unused data this port might have received. */
> discard_port_data(port);
> + remove_unused_bufs(port->in_vq);
> spin_unlock_irq(&port->inbuf_lock);
>
> spin_lock_irq(&port->outvq_lock);
> reclaim_consumed_buffers(port);
> + remove_unused_bufs(port->out_vq);
> spin_unlock_irq(&port->outvq_lock);
> }
>
> @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> struct virtqueue *vq;
>
> virtio_device_for_each_vq(portdev->vdev, vq) {
> - struct port_buffer *buf;
>
> flush_bufs(vq, true);
> - while ((buf = virtqueue_detach_unused_buf(vq)))
> - free_buf(buf, true);
> + remove_unused_bufs(vq);
> }
> portdev->vdev->config->del_vqs(portdev->vdev);
> kfree(portdev->in_vqs);
> --
> 2.20.1
^ permalink raw reply
* Re: [PATCH] virtio_console: free unused buffers with virtio port
From: Pankaj Gupta @ 2019-03-04 13:28 UTC (permalink / raw)
To: linux-kernel, virtualization; +Cc: arnd, mst, Gal Hammer, gregkh, amit, xiaohli
In-Reply-To: <20190304130511.14450-1-pagupta@redhat.com>
Adding Gal in CC [Forgot to add]
>
> The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
> deffered detaching of unused buffer to virtio device unplug time.
>
> This causes unplug/replug of single port in virtio device with an
> error "Error allocating inbufs\n". As we don't free the unused buffers
> attached with the port. Re-plug the same port tries to allocate new
> buffers in virtqueue and results in this error if queue is full.
>
> This patch removes the unused buffers in vq's when we unplug the port.
> This is the best we can do as we cannot call device_reset because virtio
> device is still active. This was the working behaviour before the change
> introduced in commit b3258ff1d6.
>
> Reported-by: Xiaohui Li <xiaohli@redhat.com>
> Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
> drivers/char/virtio_console.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fbeb71953526..5fbf2ac73111 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
> kfree(port);
> }
>
> +static void remove_unused_bufs(struct virtqueue *vq)
> +{
> + struct port_buffer *buf;
> +
> + while ((buf = virtqueue_detach_unused_buf(vq)))
> + free_buf(buf, true);
> +}
> +
> static void remove_port_data(struct port *port)
> {
> spin_lock_irq(&port->inbuf_lock);
> /* Remove unused data this port might have received. */
> discard_port_data(port);
> + remove_unused_bufs(port->in_vq);
> spin_unlock_irq(&port->inbuf_lock);
>
> spin_lock_irq(&port->outvq_lock);
> reclaim_consumed_buffers(port);
> + remove_unused_bufs(port->out_vq);
> spin_unlock_irq(&port->outvq_lock);
> }
>
> @@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
> struct virtqueue *vq;
>
> virtio_device_for_each_vq(portdev->vdev, vq) {
> - struct port_buffer *buf;
>
> flush_bufs(vq, true);
> - while ((buf = virtqueue_detach_unused_buf(vq)))
> - free_buf(buf, true);
> + remove_unused_bufs(vq);
> }
> portdev->vdev->config->del_vqs(portdev->vdev);
> kfree(portdev->in_vqs);
> --
> 2.20.1
>
>
^ permalink raw reply
* [PATCH] virtio_console: free unused buffers with virtio port
From: Pankaj Gupta @ 2019-03-04 13:05 UTC (permalink / raw)
To: linux-kernel, virtualization; +Cc: pagupta, arnd, mst, gregkh, amit, xiaohli
The commit a7a69ec0d8e4 ("virtio_console: free buffers after reset")
deffered detaching of unused buffer to virtio device unplug time.
This causes unplug/replug of single port in virtio device with an
error "Error allocating inbufs\n". As we don't free the unused buffers
attached with the port. Re-plug the same port tries to allocate new
buffers in virtqueue and results in this error if queue is full.
This patch removes the unused buffers in vq's when we unplug the port.
This is the best we can do as we cannot call device_reset because virtio
device is still active. This was the working behaviour before the change
introduced in commit b3258ff1d6.
Reported-by: Xiaohui Li <xiaohli@redhat.com>
Fixes: b3258ff1d6 ("virtio_console: free buffers after reset")
Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---
drivers/char/virtio_console.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fbeb71953526..5fbf2ac73111 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1506,15 +1506,25 @@ static void remove_port(struct kref *kref)
kfree(port);
}
+static void remove_unused_bufs(struct virtqueue *vq)
+{
+ struct port_buffer *buf;
+
+ while ((buf = virtqueue_detach_unused_buf(vq)))
+ free_buf(buf, true);
+}
+
static void remove_port_data(struct port *port)
{
spin_lock_irq(&port->inbuf_lock);
/* Remove unused data this port might have received. */
discard_port_data(port);
+ remove_unused_bufs(port->in_vq);
spin_unlock_irq(&port->inbuf_lock);
spin_lock_irq(&port->outvq_lock);
reclaim_consumed_buffers(port);
+ remove_unused_bufs(port->out_vq);
spin_unlock_irq(&port->outvq_lock);
}
@@ -1950,11 +1960,9 @@ static void remove_vqs(struct ports_device *portdev)
struct virtqueue *vq;
virtio_device_for_each_vq(portdev->vdev, vq) {
- struct port_buffer *buf;
flush_bufs(vq, true);
- while ((buf = virtqueue_detach_unused_buf(vq)))
- free_buf(buf, true);
+ remove_unused_bufs(vq);
}
portdev->vdev->config->del_vqs(portdev->vdev);
kfree(portdev->in_vqs);
--
2.20.1
^ permalink raw reply related
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Siwei Liu @ 2019-03-02 0:30 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexander Duyck, Jiri Pirko, Michael S. Tsirkin,
Samudrala, Sridhar, virtualization, liran.alon, Netdev,
si-wei liu, David Miller
In-Reply-To: <20190228170520.527ed6df@cakuba.netronome.com>
On Thu, Feb 28, 2019 at 5:05 PM Jakub Kicinski <kubakici@wp.pl> wrote:
>
> On Thu, 28 Feb 2019 16:20:28 -0800, Siwei Liu wrote:
> > On Thu, Feb 28, 2019 at 11:56 AM Jakub Kicinski wrote:
> > > On Thu, 28 Feb 2019 14:36:56 -0500, Michael S. Tsirkin wrote:
> > > > > It is a bit of a the chicken or the egg situation ;) But users can
> > > > > just blacklist, too. Anyway, I think this is far better than module
> > > > > parameters
> > > >
> > > > Sorry I'm a bit confused. What is better than what?
> > >
> > > I mean that blacklist net_failover or module param to disable
> > > net_failover and handle in user space are better than trying to solve
> > > the renaming at kernel level (either by adding module params that make
> > > the kernel rename devices or letting user space change names of running
> > > devices if they are slaves).
> >
> > Before I was aksed to revive this old mail thread, I knew the
> > discussion could end up with something like this. Yes, theoretically
> > there's a point - basically you don't believe kernel should take risk
> > in fixing the issue, so you push back the hope to something in
> > hypothesis that actually wasn't done and hard to get done in reality.
> > It's not too different than saying "hey, what you're asking for is
> > simply wrong, don't do it! Go back to modify userspace to create a
> > bond or team instead!" FWIW I want to emphasize that the debate for
> > what should be the right place to implement this failover facility:
> > userspace versus kernel, had been around for almost a decade, and no
> > real work ever happened in userspace to "standardize" this in the
> > Linux world.
>
> Let me offer you my very subjective opinion of why "no real work ever
> happened in user space". The actors who have primary interest to get
> the auto-bonding working are HW vendors trying to either convince
> customers to use SR-IOV, or being pressured by customers to make SR-IOV
> easier to consume. HW vendors hire driver developers, not user space
> developers. So the solution we arrive at is in the kernel for a non
> technical reason (Conway's law, sort of).
>
> $ cd NetworkManager/
> $ git log --pretty=format:"%ae" | \
> grep '\(mellanox\|intel\|broadcom\|netronome\)' | sort | uniq -c
> 81 andrew.zaborowski@intel.com
> 2 David.Woodhouse@intel.com
> 2 ismo.puustinen@intel.com
> 1 michael.i.doherty@intel.com
>
> Andrew works on WiFi.
>
I'm sorry, but we don't use NetworkManager in our cloud images at all.
We sufferd from lots of problems when booting from remote iSCSI disk
with NetworkManager enabled, and it looks like those issues are still
there while that's not (my subjective impression) a network config
tool mainly targeting desktop and WiFi users ever cares about. At
least a sign of lack of sufficient testing was made there.
From cloud service provider perspective, we always prefer single
central solution than speak to various distro vendors with their own
network daemons/config tools thus different solutions. It's hard to
coordicate all efforts in one place. From my personal perspetive, the
in-kernel auto-slave solution is nothing technically inferior than any
userspace implementation, and every major OS/cloud providers choose to
implement this in-kernel model for the same reason. I don't want to
argue more if there's value or not for net_failover to be in Linux
kernel, given that it's already there I think it's better to move on.
We have done extensive work in reporting (actually, fix them
internally before posting) issues to the dracut, udev,
initramfs-tools, and cloud-init community. Although as claimed the
3-netdev should be transparent to userspace in general, the reality is
opposite: the effort is nothing differenet than bring up a new type of
virutal bond than any existing userspace tool would otherwise expect
for a regular physical netdev. If there's ever concern about breaking
userspace, I bet no one ever tries to start using it. If they did they
know what I am saying. The dup MAC address setting and plugging order
are totally new to userspace that none of userspace tools fail to know
how to plumb failover interface in a proper way, if without fixing
them one or another.
-Siwei
> I have asked the NetworkManager folks to implement this feature last
> year when net_failover got dangerously close to getting merged, and
> they said they were never approached with this request before, much less
> offered code that solve it. Unfortunately before they got around to it
> net_failover was merged already, and they didn't proceed.
>
> So to my knowledge nobody ever tried to solve this in user space.
> I don't think net_failover is particularly terrible, or that renaming
> of primary in the kernel is the end of the world, but I'd appreciate if
> you could point me to efforts to solve it upstream in user space
> components, or acknowledge that nobody actually tried that.
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-03-01 13:27 UTC (permalink / raw)
To: si-wei liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Samudrala, Sridhar, virtualization, Siwei Liu, liran.alon, Netdev,
David Miller
In-Reply-To: <8a387954-1e21-947b-a5a9-c49adaea2e81@oracle.com>
On Thu, Feb 28, 2019 at 05:30:56PM -0800, si-wei liu wrote:
>
>
> On 2/28/2019 6:26 AM, Michael S. Tsirkin wrote:
> > On Thu, Feb 28, 2019 at 01:32:12AM -0800, si-wei liu wrote:
> > > > > Will the
> > > > > change break userspace further?
> > > > >
> > > > > -Siwei
> > > > Didn't you show userspace is already broken. You can't "further
> > > > break it", rename already fails.
> > > It's a race, userspace tends to give slave a user(space) desired name but
> > > sometimes may fail due to this race. Today if failover master is not up,
> > > rename would succeed anyway. While what you proposed prohibits user from
> > > providing a name in all circumstances if I understand you correctly. That's
> > > what I meant of breaking userspace further. On the other hand, you seem to
> > > tighten the kernel default naming to udev predictable names, which is
> > > derived from only recent systemd-udevd, while there exists many possible
> > > userspace naming schemes out of that. Users today who deliberately chooses
> > > to disable predictable naming (net.ifnames=0 biosdevname=0) and fall back to
> > > kernel provided names would expect the ethX pattern, with this change
> > > admin/user scripts which matches the ethX pattern could potentially break.
> > Whatever crashes with a name not matching ethX will crash on the
> > standby interface *anyway*.
> With udev predictable naming disabled they should not. It's not hard for
> user to look for device attribute to persistent the name well, in a
> consistent and reliable way.
Well that's special code for failover already. So far we just
taught userspace to skip renaming slave interfaces.
> >
> > So I think what you are saying is that someone might have already
> > written scripts and gotten them to work on v4.17 when STANDBY was
> > included and these scripts rely on ethX. Now these scripts
> > will break.
> The controversial part is the new kernel naming pattern. Initially I thought
> there shouldn't be such crazy scripts relying on the pattern, but when I
> worked on cloud-init it I realized that there's already a lot of software
> taking assumption around the 'eth0' name. In the past I've seen random
> scripts that parses the ethX name assumes (incorrectly) the name ends up
> with digits, or even the digits and name are 1:1 mapped. Of course, you can
> say these are bugs in scripts themselves.
No what I say is that they will crash on rename of standby too.
> Anyway, I'll let others in the netdev to comment on this new scheme, maybe
> that's the concern of merely myself. The good part of your proposal is that
> we can get consistent slave name, which still plays its role until we move
> towards making slave names less relevant, i.e. ideally a 1-netdev model. I
> think we both agree that the master matters more than the slave names.
> >
> > Maybe it is still early enough (just half a year passed) that the
> > number of these users would be small. So how about a kernel config
> > option and maybe a module parameter to rename the primary? People can
> > then opt in to the old broken behaviour.
> Were I could I would ask why a similar opt-in (kernel config or module
> parameter) couldn't be implemented to open up the rename restriction on
> slave, net_failover in particular. What I felt about this rename restriction
> was more because of historical reason than anything else, while net_failover
> is comparatively a new type of link that we are now designing proper use
> case it should support, and can get it shaped to whatever it fits. My
> personal view is that the slave can't be renamed when master is running is
> just implementation details that got incorrectly exposed to userspace apps
> for many years. It's old behavior with historical reason for sure, but I
> don't think this applies to net_failover.
>
> (FWIW as one previous bond maintainer for another OS, we relieved the rename
> restriction slaves 13 year ago, while no single complaint or issue was ever
> raised because of this change over the years, neither from the customers of
> tens of millions of installation base, nor the FOSS software running atop.
> Of course, Linux is different so that experience doesn't count.)
>
> Thanks,
> -Siwei
>
^ 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