* Re: [PATCH 7/8] drm: Nuke drm_atomic_helper_connector_dpms
From: Noralf Trønnes @ 2017-07-26 19:00 UTC (permalink / raw)
To: Daniel Vetter, Intel Graphics Development
Cc: Martyn Welch, Neil Armstrong, David Airlie, nouveau,
DRI Development, virtualization, Andrzej Hajda, Tomi Valkeinen,
Laurent Pinchart, Benjamin Gaignard, Romain Perier, Daniel Vetter,
Jyri Sarha, Heiko Stuebner, Marek Vasut, linux-renesas-soc,
Archit Taneja, Lars-Peter Clausen, Joonyoung Shim, Kevin Hilman,
Alexey Brodkin, Russell King, Krzy
In-Reply-To: <20170725080122.20548-8-daniel.vetter@ffwll.ch>
Den 25.07.2017 10.01, skrev Daniel Vetter:
> It's dead code, the core handles all this directly now.
>
> The only special case is nouveau and tda988x which used one function
> for both legacy modeset code and -nv50 atomic world instead of 2
> vtables. But amounts to exactly the same.
>
> v2: Rebase over the panel/brideg refactorings in stm/ltdc.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
...
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> index ec43fb7ad9e4..79b6687977d3 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c
> @@ -71,7 +71,6 @@ static void tinydrm_connector_destroy(struct drm_connector *connector)
> }
>
> static const struct drm_connector_funcs tinydrm_connector_funcs = {
> - .dpms = drm_atomic_helper_connector_dpms,
> .reset = drm_atomic_helper_connector_reset,
> .detect = tinydrm_connector_detect,
> .fill_modes = drm_helper_probe_single_connector_modes,
Acked-by: Noralf Trønnes <noralf@tronnes.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v12 5/8] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-07-26 17:02 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <59781119.8010200@intel.com>
On Wed, Jul 26, 2017 at 11:48:41AM +0800, Wei Wang wrote:
> On 07/23/2017 09:45 AM, Michael S. Tsirkin wrote:
> > On Fri, Jul 14, 2017 at 03:12:43PM +0800, Wei Wang wrote:
> > > On 07/14/2017 04:19 AM, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 13, 2017 at 03:42:35PM +0800, Wei Wang wrote:
> > > > > On 07/12/2017 09:56 PM, Michael S. Tsirkin wrote:
> > > > > > So the way I see it, there are several issues:
> > > > > >
> > > > > > - internal wait - forces multiple APIs like kick/kick_sync
> > > > > > note how kick_sync can fail but your code never checks return code
> > > > > > - need to re-write the last descriptor - might not work
> > > > > > for alternative layouts which always expose descriptors
> > > > > > immediately
> > > > > Probably it wasn't clear. Please let me explain the two functions here:
> > > > >
> > > > > 1) virtqueue_add_chain_desc(vq, head_id, prev_id,..):
> > > > > grabs a desc from the vq and inserts it to the chain tail (which is indexed
> > > > > by
> > > > > prev_id, probably better to call it tail_id). Then, the new added desc
> > > > > becomes
> > > > > the tail (i.e. the last desc). The _F_NEXT flag is cleared for each desc
> > > > > when it's
> > > > > added to the chain, and set when another desc comes to follow later.
> > > > And this only works if there are multiple rings like
> > > > avail + descriptor ring.
> > > > It won't work e.g. with the proposed new layout where
> > > > writing out a descriptor exposes it immediately.
> > > I think it can support the 1.1 proposal, too. But before getting
> > > into that, I think we first need to deep dive into the implementation
> > > and usage of _first/next/last. The usage would need to lock the vq
> > > from the first to the end (otherwise, the returned info about the number
> > > of available desc in the vq, i.e. num_free, would be invalid):
> > >
> > > lock(vq);
> > > add_first();
> > > add_next();
> > > add_last();
> > > unlock(vq);
> > >
> > > However, I think the case isn't this simple, since we need to check more
> > > things
> > > after each add_xx() step. For example, if only one entry is available at the
> > > time
> > > we start to use the vq, that is, num_free is 0 after add_first(), we
> > > wouldn't be
> > > able to add_next and add_last. So, it would work like this:
> > >
> > > start:
> > > ...get free page block..
> > > lock(vq)
> > > retry:
> > > ret = add_first(..,&num_free,);
> > > if(ret == -ENOSPC) {
> > > goto retry;
> > > } else if (!num_free) {
> > > add_chain_head();
> > > unlock(vq);
> > > kick & wait;
> > > goto start;
> > > }
> > > next_one:
> > > ...get free page block..
> > > add_next(..,&num_free,);
> > > if (!num_free) {
> > > add_chain_head();
> > > unlock(vq);
> > > kick & wait;
> > > goto start;
> > > } if (num_free == 1) {
> > > ...get free page block..
> > > add_last(..);
> > > unlock(vq);
> > > kick & wait;
> > > goto start;
> > > } else {
> > > goto next_one;
> > > }
> > >
> > > The above seems unnecessary to me to have three different APIs.
> > > That's the reason to combine them into one virtqueue_add_chain_desc().
> > >
> > > -- or, do you have a different thought about using the three APIs?
> > >
> > >
> > > Implementation Reference:
> > >
> > > struct desc_iterator {
> > > unsigned int head;
> > > unsigned int tail;
> > > };
> > >
> > > add_first(*vq, *desc_iterator, *num_free, ..)
> > > {
> > > if (vq->vq.num_free < 1)
> > > return -ENOSPC;
> > > get_desc(&desc_id);
> > > desc[desc_id].flag &= ~_F_NEXT;
> > > desc_iterator->head = desc_id
> > > desc_iterator->tail = desc_iterator->head;
> > > *num_free = vq->vq.num_free;
> > > }
> > >
> > > add_next(vq, desc_iterator, *num_free,..)
> > > {
> > > get_desc(&desc_id);
> > > desc[desc_id].flag &= ~_F_NEXT;
> > > desc[desc_iterator.tail].next = desc_id;
> > > desc[desc_iterator->tail].flag |= _F_NEXT;
> > > desc_iterator->tail = desc_id;
> > > *num_free = vq->vq.num_free;
> > > }
> > >
> > > add_last(vq, desc_iterator,..)
> > > {
> > > get_desc(&desc_id);
> > > desc[desc_id].flag &= ~_F_NEXT;
> > > desc[desc_iterator.tail].next = desc_id;
> > > desc_iterator->tail = desc_id;
> > >
> > > add_chain_head(); // put the desc_iterator.head to the ring
> > > }
> > >
> > >
> > > Best,
> > > Wei
> > OK I thought this over. While we might need these new APIs in
> > the future, I think that at the moment, there's a way to implement
> > this feature that is significantly simpler. Just add each s/g
> > as a separate input buffer.
>
>
> Should it be an output buffer?
Hypervisor overwrites these pages with zeroes. Therefore it is
writeable by device: DMA_FROM_DEVICE.
> I think output means from the
> driver to device (i.e. DMA_TO_DEVICE).
This part is correct I believe.
> >
> > This needs zero new APIs.
> >
> > I know that follow-up patches need to add a header in front
> > so you might be thinking: how am I going to add this
> > header? The answer is quite simple - add it as a separate
> > out header.
> >
> > Host will be able to distinguish between header and pages
> > by looking at the direction, and - should we want to add
> > IN data to header - additionally size (<4K => header).
>
>
> I think this works fine when the cmdq is only used for
> reporting the unused pages.
> It would be an issue
> if there are other usages (e.g. report memory statistics)
> interleaving. I think one solution would be to lock the cmdq until
> a cmd usage is done ((e.g. all the unused pages are reported) ) -
> in this case, the periodically updated guest memory statistics
> may be delayed for a while occasionally when live migration starts.
> Would this be acceptable? If not, probably we can have the cmdq
> for one usage only.
>
>
> Best,
> Wei
OK I see, I think the issue is that reporting free pages
was structured like stats. Let's split it -
send pages on e.g. free_vq, get commands on vq shared with
stats.
--
MST
^ permalink raw reply
* [PULL] vhost: fixes, cleanups
From: Michael S. Tsirkin @ 2017-07-26 16:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: drjones, kvm, mst, netdev, bigeasy, liang.z.li, linux-kernel,
virtualization, andriy.shevchenko
All's quiet here this cycle, just some minor fixes. A block printk
message bugfix is still pending for the next pull, didn't want to delay
this one as people are getting impatient.
The following changes since commit 520eccdfe187591a51ea9ab4c1a024ae4d0f68d9:
Linux 4.13-rc2 (2017-07-23 16:15:17 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to cfa0ebc9d6d6308564f5174ecb655b9d504b2be5:
virtio-net: fix module unloading (2017-07-25 16:37:36 +0300)
----------------------------------------------------------------
virtio: fixes, cleanups
Fixes some minor issues all over the codebase.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Andrew Jones (1):
virtio-net: fix module unloading
Andy Shevchenko (1):
virtio_blk: Use sysfs_match_string() helper
Liang Li (1):
virtio-balloon: deflate via a page list
Wei Wang (1):
virtio-balloon: coding format cleanup
drivers/block/virtio_blk.c | 7 ++-----
drivers/net/virtio_net.c | 2 +-
drivers/virtio/virtio_balloon.c | 28 ++++++++++++----------------
3 files changed, 15 insertions(+), 22 deletions(-)
^ permalink raw reply
* Re: [PATCH net] Revert "vhost: cache used event for better performance"
From: Michael S. Tsirkin @ 2017-07-26 16:08 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <6b3a9a98-c095-1729-3528-dd521f136797@redhat.com>
On Wed, Jul 26, 2017 at 09:37:15PM +0800, Jason Wang wrote:
>
>
> On 2017年07月26日 21:18, Jason Wang wrote:
> >
> >
> > On 2017年07月26日 20:57, Michael S. Tsirkin wrote:
> > > On Wed, Jul 26, 2017 at 04:03:17PM +0800, Jason Wang wrote:
> > > > This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
> > > > was reported to break vhost_net. We want to cache used event and use
> > > > it to check for notification. We try to valid cached used event by
> > > > checking whether or not it was ahead of new, but this is not correct
> > > > all the time, it could be stale and there's no way to know about this.
> > > >
> > > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > > Could you supply a bit more data here please? How does it get stale?
> > > What does guest need to do to make it stale? This will be helpful if
> > > anyone wants to bring it back, or if we want to extend the protocol.
> > >
> >
> > The problem we don't know whether or not guest has published a new used
> > event. The check vring_need_event(vq->last_used_event, new + vq->num,
> > new) is not sufficient to check for this.
> >
> > Thanks
>
> More notes, the previous assumption is that we don't move used event back,
> but this could happen in fact if idx is wrapper around.
You mean if the 16 bit index wraps around after 64K entries.
Makes sense.
> Will repost and add
> this into commit log.
>
> Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] Revert "vhost: cache used event for better performance"
From: Michael S. Tsirkin @ 2017-07-26 15:21 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <117542fc-eef9-c043-7c9e-daafceb7db4e@redhat.com>
On Wed, Jul 26, 2017 at 09:18:02PM +0800, Jason Wang wrote:
>
>
> On 2017年07月26日 20:57, Michael S. Tsirkin wrote:
> > On Wed, Jul 26, 2017 at 04:03:17PM +0800, Jason Wang wrote:
> > > This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
> > > was reported to break vhost_net. We want to cache used event and use
> > > it to check for notification. We try to valid cached used event by
> > > checking whether or not it was ahead of new, but this is not correct
> > > all the time, it could be stale and there's no way to know about this.
> > >
> > > Signed-off-by: Jason Wang<jasowang@redhat.com>
> > Could you supply a bit more data here please? How does it get stale?
> > What does guest need to do to make it stale? This will be helpful if
> > anyone wants to bring it back, or if we want to extend the protocol.
> >
>
> The problem we don't know whether or not guest has published a new used
> event. The check vring_need_event(vq->last_used_event, new + vq->num, new)
> is not sufficient to check for this.
>
> Thanks
Figured it out after an offline discussion.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH] virtio_blk: fix incorrect message when disk is resized
From: Stefan Hajnoczi @ 2017-07-26 14:32 UTC (permalink / raw)
To: linux-kernel; +Cc: virtualization, Stefan Hajnoczi, Michael S. Tsirkin
The message printed on disk resize is incorrect. The following is
printed when resizing to 2 GiB:
$ truncate -s 1G test.img
$ qemu -device virtio-blk-pci,logical_block_size=4096,...
(qemu) block_resize drive1 2G
virtio_blk virtio0: new size: 4194304 4096-byte logical blocks (17.2 GB/16.0 GiB)
The virtio_blk capacity config field is in 512-byte sector units
regardless of logical_block_size as per the VIRTIO specification.
Therefore the message should read:
virtio_blk virtio0: new size: 524288 4096-byte logical blocks (2.15 GB/2.0 GiB)
Note that this only affects the printed message. Thankfully the actual
block device has the correct size because the block layer expects
capacity in sectors.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
drivers/block/virtio_blk.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4e02aa5fdac0..69a2d1748743 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -381,6 +381,7 @@ static void virtblk_config_changed_work(struct work_struct *work)
struct request_queue *q = vblk->disk->queue;
char cap_str_2[10], cap_str_10[10];
char *envp[] = { "RESIZE=1", NULL };
+ unsigned long long nblocks;
u64 capacity;
/* Host must always specify the capacity. */
@@ -393,16 +394,19 @@ static void virtblk_config_changed_work(struct work_struct *work)
capacity = (sector_t)-1;
}
- string_get_size(capacity, queue_logical_block_size(q),
+ nblocks = DIV_ROUND_UP_ULL(capacity, queue_logical_block_size(q) >> 9);
+
+ string_get_size(nblocks, queue_logical_block_size(q),
STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
- string_get_size(capacity, queue_logical_block_size(q),
+ string_get_size(nblocks, queue_logical_block_size(q),
STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
dev_notice(&vdev->dev,
- "new size: %llu %d-byte logical blocks (%s/%s)\n",
- (unsigned long long)capacity,
- queue_logical_block_size(q),
- cap_str_10, cap_str_2);
+ "new size: %llu %d-byte logical blocks (%s/%s)\n",
+ nblocks,
+ queue_logical_block_size(q),
+ cap_str_10,
+ cap_str_2);
set_capacity(vblk->disk, capacity);
revalidate_disk(vblk->disk);
--
2.13.3
^ permalink raw reply related
* Re: [PATCH net] Revert "vhost: cache used event for better performance"
From: Jason Wang @ 2017-07-26 13:37 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <117542fc-eef9-c043-7c9e-daafceb7db4e@redhat.com>
On 2017年07月26日 21:18, Jason Wang wrote:
>
>
> On 2017年07月26日 20:57, Michael S. Tsirkin wrote:
>> On Wed, Jul 26, 2017 at 04:03:17PM +0800, Jason Wang wrote:
>>> This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
>>> was reported to break vhost_net. We want to cache used event and use
>>> it to check for notification. We try to valid cached used event by
>>> checking whether or not it was ahead of new, but this is not correct
>>> all the time, it could be stale and there's no way to know about this.
>>>
>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> Could you supply a bit more data here please? How does it get stale?
>> What does guest need to do to make it stale? This will be helpful if
>> anyone wants to bring it back, or if we want to extend the protocol.
>>
>
> The problem we don't know whether or not guest has published a new
> used event. The check vring_need_event(vq->last_used_event, new +
> vq->num, new) is not sufficient to check for this.
>
> Thanks
More notes, the previous assumption is that we don't move used event
back, but this could happen in fact if idx is wrapper around. Will
repost and add this into commit log.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] Revert "vhost: cache used event for better performance"
From: Jason Wang @ 2017-07-26 13:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20170726155435-mutt-send-email-mst@kernel.org>
On 2017年07月26日 20:57, Michael S. Tsirkin wrote:
> On Wed, Jul 26, 2017 at 04:03:17PM +0800, Jason Wang wrote:
>> This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
>> was reported to break vhost_net. We want to cache used event and use
>> it to check for notification. We try to valid cached used event by
>> checking whether or not it was ahead of new, but this is not correct
>> all the time, it could be stale and there's no way to know about this.
>>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> Could you supply a bit more data here please? How does it get stale?
> What does guest need to do to make it stale? This will be helpful if
> anyone wants to bring it back, or if we want to extend the protocol.
>
The problem we don't know whether or not guest has published a new used
event. The check vring_need_event(vq->last_used_event, new + vq->num,
new) is not sufficient to check for this.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] Revert "vhost: cache used event for better performance"
From: Michael S. Tsirkin @ 2017-07-26 12:57 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1501056197-3368-1-git-send-email-jasowang@redhat.com>
On Wed, Jul 26, 2017 at 04:03:17PM +0800, Jason Wang wrote:
> This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
> was reported to break vhost_net. We want to cache used event and use
> it to check for notification. We try to valid cached used event by
> checking whether or not it was ahead of new, but this is not correct
> all the time, it could be stale and there's no way to know about this.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Could you supply a bit more data here please? How does it get stale?
What does guest need to do to make it stale? This will be helpful if
anyone wants to bring it back, or if we want to extend the protocol.
> ---
> drivers/vhost/vhost.c | 28 ++++++----------------------
> drivers/vhost/vhost.h | 3 ---
> 2 files changed, 6 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index e4613a3..9cb3f72 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -308,7 +308,6 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> vq->avail = NULL;
> vq->used = NULL;
> vq->last_avail_idx = 0;
> - vq->last_used_event = 0;
> vq->avail_idx = 0;
> vq->last_used_idx = 0;
> vq->signalled_used = 0;
> @@ -1402,7 +1401,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
> r = -EINVAL;
> break;
> }
> - vq->last_avail_idx = vq->last_used_event = s.num;
> + vq->last_avail_idx = s.num;
> /* Forget the cached index value. */
> vq->avail_idx = vq->last_avail_idx;
> break;
> @@ -2241,6 +2240,10 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> __u16 old, new;
> __virtio16 event;
> bool v;
> + /* Flush out used index updates. This is paired
> + * with the barrier that the Guest executes when enabling
> + * interrupts. */
> + smp_mb();
>
> if (vhost_has_feature(vq, VIRTIO_F_NOTIFY_ON_EMPTY) &&
> unlikely(vq->avail_idx == vq->last_avail_idx))
> @@ -2248,10 +2251,6 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>
> if (!vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {
> __virtio16 flags;
> - /* Flush out used index updates. This is paired
> - * with the barrier that the Guest executes when enabling
> - * interrupts. */
> - smp_mb();
> if (vhost_get_avail(vq, flags, &vq->avail->flags)) {
> vq_err(vq, "Failed to get flags");
> return true;
> @@ -2266,26 +2265,11 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> if (unlikely(!v))
> return true;
>
> - /* We're sure if the following conditions are met, there's no
> - * need to notify guest:
> - * 1) cached used event is ahead of new
> - * 2) old to new updating does not cross cached used event. */
> - if (vring_need_event(vq->last_used_event, new + vq->num, new) &&
> - !vring_need_event(vq->last_used_event, new, old))
> - return false;
> -
> - /* Flush out used index updates. This is paired
> - * with the barrier that the Guest executes when enabling
> - * interrupts. */
> - smp_mb();
> -
> if (vhost_get_avail(vq, event, vhost_used_event(vq))) {
> vq_err(vq, "Failed to get used event idx");
> return true;
> }
> - vq->last_used_event = vhost16_to_cpu(vq, event);
> -
> - return vring_need_event(vq->last_used_event, new, old);
> + return vring_need_event(vhost16_to_cpu(vq, event), new, old);
> }
>
> /* This actually signals the guest, using eventfd. */
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index f720958..bb7c29b 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -115,9 +115,6 @@ struct vhost_virtqueue {
> /* Last index we used. */
> u16 last_used_idx;
>
> - /* Last used evet we've seen */
> - u16 last_used_event;
> -
> /* Used flags */
> u16 used_flags;
>
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH] virtio-net: fix module unloading
From: Michael S. Tsirkin @ 2017-07-26 12:50 UTC (permalink / raw)
To: Jason Wang
Cc: Andrew Jones, linux-kernel, Sebastian Andrzej Siewior,
virtualization
In-Reply-To: <37d51fa3-21e4-ae55-6d5b-6ade97ae4bc8@redhat.com>
On Wed, Jul 26, 2017 at 11:52:07AM +0800, Jason Wang wrote:
>
>
> On 2017年07月24日 21:38, Andrew Jones wrote:
> > Unregister the driver before removing multi-instance hotplug
> > callbacks. This order avoids the warning issued from
> > __cpuhp_remove_state_cpuslocked when the number of remaining
> > instances isn't yet zero.
> >
> > Fixes: 8017c279196a ("net/virtio-net: Convert to hotplug state machine")
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > drivers/net/virtio_net.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 99a26a9efec1..f41ab0ea942a 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2743,9 +2743,9 @@ module_init(virtio_net_driver_init);
> > static __exit void virtio_net_driver_exit(void)
> > {
> > + unregister_virtio_driver(&virtio_net_driver);
> > cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
> > cpuhp_remove_multi_state(virtionet_online);
> > - unregister_virtio_driver(&virtio_net_driver);
> > }
> > module_exit(virtio_net_driver_exit);
>
> Acked-by: Jason Wang <jasowang@redhat.com>
Thanks for the review!
I merged it before the tag and don't want to rebase.
Sorry about that.
--
MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* RE: [PATCH v12 6/8] mm: support reporting free page blocks
From: Wang, Wei W @ 2017-07-26 12:47 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
amit.shah@redhat.com, kvm@vger.kernel.org, Michael S. Tsirkin,
linux-kernel@vger.kernel.org, liliang.opensource@gmail.com,
qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <20170726115506.GM2981@dhcp22.suse.cz>
On Wednesday, July 26, 2017 7:55 PM, Michal Hocko wrote:
> On Wed 26-07-17 19:44:23, Wei Wang wrote:
> [...]
> > I thought about it more. Probably we can use the callback function
> > with a little change like this:
> >
> > void walk_free_mem(void *opaque1, void (*visit)(void *opaque2,
> > unsigned long pfn,
> > unsigned long nr_pages))
> > {
> > ...
> > for_each_populated_zone(zone) {
> > for_each_migratetype_order(order, type) {
> > report_unused_page_block(zone, order, type,
> > &page); // from patch 6
> > pfn = page_to_pfn(page);
> > visit(opaque1, pfn, 1 << order);
> > }
> > }
> > }
> >
> > The above function scans all the free list and directly sends each
> > free page block to the hypervisor via the virtio_balloon callback
> > below. No need to implement a bitmap.
> >
> > In virtio-balloon, we have the callback:
> > void *virtio_balloon_report_unused_pages(void *opaque, unsigned long
> > pfn, unsigned long nr_pages) {
> > struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> > ...put the free page block to the the ring of vb; }
> >
> >
> > What do you think?
>
> I do not mind conveying a context to the callback. I would still prefer
> to keep the original min_order to check semantic though. Why? Well,
> it doesn't make much sense to scan low order free blocks all the time
> because they are simply too volatile. Larger blocks tend to surivive for
> longer. So I assume you would only care about larger free blocks. This
> will also make the call cheaper.
> --
OK, I will keep min order there in the next version.
Best,
Wei
^ permalink raw reply
* Re: [PATCH net] Revert "vhost: cache used event for better performance"
From: Jason Wang @ 2017-07-26 11:56 UTC (permalink / raw)
To: Christian Borntraeger, mst; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <da300d74-ca7a-a636-0ed6-6aa78e6a5510@de.ibm.com>
On 2017年07月26日 18:53, Christian Borntraeger wrote:
> On 07/26/2017 10:03 AM, Jason Wang wrote:
>> This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
>> was reported to break vhost_net. We want to cache used event and use
>> it to check for notification. We try to valid cached used event by
>> checking whether or not it was ahead of new, but this is not correct
>> all the time, it could be stale and there's no way to know about this.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> This would then qualify for stable ?
>
Yes it is.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Michal Hocko @ 2017-07-26 11:55 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
amit.shah@redhat.com, kvm@vger.kernel.org, Michael S. Tsirkin,
linux-kernel@vger.kernel.org, liliang.opensource@gmail.com,
qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <59788097.6010402@intel.com>
On Wed 26-07-17 19:44:23, Wei Wang wrote:
[...]
> I thought about it more. Probably we can use the callback function with a
> little change like this:
>
> void walk_free_mem(void *opaque1, void (*visit)(void *opaque2, unsigned long
> pfn,
> unsigned long nr_pages))
> {
> ...
> for_each_populated_zone(zone) {
> for_each_migratetype_order(order, type) {
> report_unused_page_block(zone, order, type, &page);
> // from patch 6
> pfn = page_to_pfn(page);
> visit(opaque1, pfn, 1 << order);
> }
> }
> }
>
> The above function scans all the free list and directly sends each free page
> block to the
> hypervisor via the virtio_balloon callback below. No need to implement a
> bitmap.
>
> In virtio-balloon, we have the callback:
> void *virtio_balloon_report_unused_pages(void *opaque, unsigned long pfn,
> unsigned long nr_pages)
> {
> struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> ...put the free page block to the the ring of vb;
> }
>
>
> What do you think?
I do not mind conveying a context to the callback. I would still prefer
to keep the original min_order to check semantic though. Why? Well,
it doesn't make much sense to scan low order free blocks all the time
because they are simply too volatile. Larger blocks tend to surivive for
longer. So I assume you would only care about larger free blocks. This
will also make the call cheaper.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Wei Wang @ 2017-07-26 11:44 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
amit.shah@redhat.com, kvm@vger.kernel.org, Michael S. Tsirkin,
linux-kernel@vger.kernel.org, liliang.opensource@gmail.com,
qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <20170726102458.GH2981@dhcp22.suse.cz>
On 07/26/2017 06:24 PM, Michal Hocko wrote:
> On Wed 26-07-17 10:22:23, Wei Wang wrote:
>> On 07/25/2017 10:53 PM, Michal Hocko wrote:
>>> On Tue 25-07-17 14:47:16, Wang, Wei W wrote:
>>>> On Tuesday, July 25, 2017 8:42 PM, hal Hocko wrote:
>>>>> On Tue 25-07-17 19:56:24, Wei Wang wrote:
>>>>>> On 07/25/2017 07:25 PM, Michal Hocko wrote:
>>>>>>> On Tue 25-07-17 17:32:00, Wei Wang wrote:
>>>>>>>> On 07/24/2017 05:00 PM, Michal Hocko wrote:
>>>>>>>>> On Wed 19-07-17 20:01:18, Wei Wang wrote:
>>>>>>>>>> On 07/19/2017 04:13 PM, Michal Hocko wrote:
>>>>>>>>> [...
>>>>>> We don't need to do the pfn walk in the guest kernel. When the API
>>>>>> reports, for example, a 2MB free page block, the API caller offers to
>>>>>> the hypervisor the base address of the page block, and size=2MB, to
>>>>>> the hypervisor.
>>>>> So you want to skip pfn walks by regularly calling into the page allocator to
>>>>> update your bitmap. If that is the case then would an API that would allow you
>>>>> to update your bitmap via a callback be s sufficient? Something like
>>>>> void walk_free_mem(int node, int min_order,
>>>>> void (*visit)(unsigned long pfn, unsigned long nr_pages))
>>>>>
>>>>> The function will call the given callback for each free memory block on the given
>>>>> node starting from the given min_order. The callback will be strictly an atomic
>>>>> and very light context. You can update your bitmap from there.
>>>> I would need to introduce more about the background here:
>>>> The hypervisor and the guest live in their own address space. The hypervisor's bitmap
>>>> isn't seen by the guest. I think we also wouldn't be able to give a callback function
>>> >from the hypervisor to the guest in this case.
>>> How did you plan to use your original API which export struct page array
>>> then?
>>
>> That's where the virtio-balloon driver comes in. It uses a shared ring
>> mechanism to
>> send the guest memory info to the hypervisor.
>>
>> We didn't expose the struct page array from the guest to the hypervisor. For
>> example, when
>> a 2MB free page block is reported from the free page list, the info put on
>> the ring is just
>> (base address of the 2MB continuous memory, size=2M).
> So what exactly prevents virtio-balloon from using the above proposed
> callback mechanism and export what is needed to the hypervisor?
I thought about it more. Probably we can use the callback function with
a little change like this:
void walk_free_mem(void *opaque1, void (*visit)(void *opaque2, unsigned
long pfn,
unsigned long nr_pages))
{
...
for_each_populated_zone(zone) {
for_each_migratetype_order(order, type) {
report_unused_page_block(zone, order, type,
&page); // from patch 6
pfn = page_to_pfn(page);
visit(opaque1, pfn, 1 << order);
}
}
}
The above function scans all the free list and directly sends each free
page block to the
hypervisor via the virtio_balloon callback below. No need to implement a
bitmap.
In virtio-balloon, we have the callback:
void *virtio_balloon_report_unused_pages(void *opaque, unsigned long pfn,
unsigned long nr_pages)
{
struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
...put the free page block to the the ring of vb;
}
What do you think?
Best,
Wei
^ permalink raw reply
* Re: [PATCH net] Revert "vhost: cache used event for better performance"
From: Christian Borntraeger @ 2017-07-26 10:53 UTC (permalink / raw)
To: Jason Wang, mst; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1501056197-3368-1-git-send-email-jasowang@redhat.com>
On 07/26/2017 10:03 AM, Jason Wang wrote:
> This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
> was reported to break vhost_net. We want to cache used event and use
> it to check for notification. We try to valid cached used event by
> checking whether or not it was ahead of new, but this is not correct
> all the time, it could be stale and there's no way to know about this.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
This would then qualify for stable ?
^ permalink raw reply
* (unknown),
From: Solen win2 @ 2017-07-26 10:32 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 4 bytes --]
all
[-- Attachment #1.2: Type: text/html, Size: 79 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 v12 6/8] mm: support reporting free page blocks
From: Michal Hocko @ 2017-07-26 10:24 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
amit.shah@redhat.com, kvm@vger.kernel.org, Michael S. Tsirkin,
linux-kernel@vger.kernel.org, liliang.opensource@gmail.com,
qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <5977FCDF.7040606@intel.com>
On Wed 26-07-17 10:22:23, Wei Wang wrote:
> On 07/25/2017 10:53 PM, Michal Hocko wrote:
> >On Tue 25-07-17 14:47:16, Wang, Wei W wrote:
> >>On Tuesday, July 25, 2017 8:42 PM, hal Hocko wrote:
> >>>On Tue 25-07-17 19:56:24, Wei Wang wrote:
> >>>>On 07/25/2017 07:25 PM, Michal Hocko wrote:
> >>>>>On Tue 25-07-17 17:32:00, Wei Wang wrote:
> >>>>>>On 07/24/2017 05:00 PM, Michal Hocko wrote:
> >>>>>>>On Wed 19-07-17 20:01:18, Wei Wang wrote:
> >>>>>>>>On 07/19/2017 04:13 PM, Michal Hocko wrote:
> >>>>>>>[...
> >>>>We don't need to do the pfn walk in the guest kernel. When the API
> >>>>reports, for example, a 2MB free page block, the API caller offers to
> >>>>the hypervisor the base address of the page block, and size=2MB, to
> >>>>the hypervisor.
> >>>So you want to skip pfn walks by regularly calling into the page allocator to
> >>>update your bitmap. If that is the case then would an API that would allow you
> >>>to update your bitmap via a callback be s sufficient? Something like
> >>> void walk_free_mem(int node, int min_order,
> >>> void (*visit)(unsigned long pfn, unsigned long nr_pages))
> >>>
> >>>The function will call the given callback for each free memory block on the given
> >>>node starting from the given min_order. The callback will be strictly an atomic
> >>>and very light context. You can update your bitmap from there.
> >>I would need to introduce more about the background here:
> >>The hypervisor and the guest live in their own address space. The hypervisor's bitmap
> >>isn't seen by the guest. I think we also wouldn't be able to give a callback function
> >>from the hypervisor to the guest in this case.
> >How did you plan to use your original API which export struct page array
> >then?
>
>
> That's where the virtio-balloon driver comes in. It uses a shared ring
> mechanism to
> send the guest memory info to the hypervisor.
>
> We didn't expose the struct page array from the guest to the hypervisor. For
> example, when
> a 2MB free page block is reported from the free page list, the info put on
> the ring is just
> (base address of the 2MB continuous memory, size=2M).
So what exactly prevents virtio-balloon from using the above proposed
callback mechanism and export what is needed to the hypervisor?
> >>>This would address my main concern that the allocator internals would get
> >>>outside of the allocator proper.
> >>What issue would it have to expose the internal, for_each_zone()?
> >zone is a MM internal concept. No code outside of the MM proper should
> >really care about zones.
>
> I think this is also what Andrew suggested in the previous discussion:
> https://lkml.org/lkml/2017/3/16/951
>
> Move the code to virtio-balloon and a little layering violation seems
> acceptable.
Andrew didn't suggest to expose zones to modules. If I read his words
properly he said that a functionality which "provides a snapshot of the
present system unused pages" is just too specific for virtio usecase
to be a generic function and as such it should be in virtio. I tend
to agree. Even the proposed callback API is a layer violation. We
should just make sure that the API is not inherently dangerous. That
is why I have chosen to give only pfn and nr_pages to the caller. Sure
somebody could argue that the caller could do pfn_to_page and do nasty
things. That would be a fair argument but nothing really prevents
anybody to do th pfn walk already so there shouldn't inherently more
risk. We can document what we expect from the API user and that would be
much easier to describe than struct page API IMHO.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [PATCH net] Revert "vhost: cache used event for better performance"
From: Jason Wang @ 2017-07-26 8:03 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, linux-kernel, kvm, virtualization
This reverts commit 809ecb9bca6a9424ccd392d67e368160f8b76c92. Since it
was reported to break vhost_net. We want to cache used event and use
it to check for notification. We try to valid cached used event by
checking whether or not it was ahead of new, but this is not correct
all the time, it could be stale and there's no way to know about this.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 28 ++++++----------------------
drivers/vhost/vhost.h | 3 ---
2 files changed, 6 insertions(+), 25 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e4613a3..9cb3f72 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -308,7 +308,6 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->avail = NULL;
vq->used = NULL;
vq->last_avail_idx = 0;
- vq->last_used_event = 0;
vq->avail_idx = 0;
vq->last_used_idx = 0;
vq->signalled_used = 0;
@@ -1402,7 +1401,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
r = -EINVAL;
break;
}
- vq->last_avail_idx = vq->last_used_event = s.num;
+ vq->last_avail_idx = s.num;
/* Forget the cached index value. */
vq->avail_idx = vq->last_avail_idx;
break;
@@ -2241,6 +2240,10 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
__u16 old, new;
__virtio16 event;
bool v;
+ /* Flush out used index updates. This is paired
+ * with the barrier that the Guest executes when enabling
+ * interrupts. */
+ smp_mb();
if (vhost_has_feature(vq, VIRTIO_F_NOTIFY_ON_EMPTY) &&
unlikely(vq->avail_idx == vq->last_avail_idx))
@@ -2248,10 +2251,6 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (!vhost_has_feature(vq, VIRTIO_RING_F_EVENT_IDX)) {
__virtio16 flags;
- /* Flush out used index updates. This is paired
- * with the barrier that the Guest executes when enabling
- * interrupts. */
- smp_mb();
if (vhost_get_avail(vq, flags, &vq->avail->flags)) {
vq_err(vq, "Failed to get flags");
return true;
@@ -2266,26 +2265,11 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
if (unlikely(!v))
return true;
- /* We're sure if the following conditions are met, there's no
- * need to notify guest:
- * 1) cached used event is ahead of new
- * 2) old to new updating does not cross cached used event. */
- if (vring_need_event(vq->last_used_event, new + vq->num, new) &&
- !vring_need_event(vq->last_used_event, new, old))
- return false;
-
- /* Flush out used index updates. This is paired
- * with the barrier that the Guest executes when enabling
- * interrupts. */
- smp_mb();
-
if (vhost_get_avail(vq, event, vhost_used_event(vq))) {
vq_err(vq, "Failed to get used event idx");
return true;
}
- vq->last_used_event = vhost16_to_cpu(vq, event);
-
- return vring_need_event(vq->last_used_event, new, old);
+ return vring_need_event(vhost16_to_cpu(vq, event), new, old);
}
/* This actually signals the guest, using eventfd. */
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index f720958..bb7c29b 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -115,9 +115,6 @@ struct vhost_virtqueue {
/* Last index we used. */
u16 last_used_idx;
- /* Last used evet we've seen */
- u16 last_used_event;
-
/* Used flags */
u16 used_flags;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next] virtio-net: mark PM functions as __maybe_unused
From: David Miller @ 2017-07-26 4:23 UTC (permalink / raw)
To: arnd
Cc: willemb, mst, netdev, john.fastabend, linux-kernel,
virtualization, edumazet, bigeasy
In-Reply-To: <20170725153600.211694-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 25 Jul 2017 17:35:50 +0200
> After removing the reset function, the freeze and restore functions
> are now unused when CONFIG_PM_SLEEP is disabled:
>
> drivers/net/virtio_net.c:1881:12: error: 'virtnet_restore_up' defined but not used [-Werror=unused-function]
> static int virtnet_restore_up(struct virtio_device *vdev)
> drivers/net/virtio_net.c:1859:13: error: 'virtnet_freeze_down' defined but not used [-Werror=unused-function]
> static void virtnet_freeze_down(struct virtio_device *vdev)
>
> A more robust way to do this is to remove the #ifdef around the callers
> and instead mark them as __maybe_unused. The compiler will now just
> silently drop the unused code.
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] virtio-net: fix module unloading
From: Jason Wang @ 2017-07-26 3:52 UTC (permalink / raw)
To: Andrew Jones, linux-kernel; +Cc: Sebastian Andrzej Siewior, virtualization, mst
In-Reply-To: <20170724133832.27364-1-drjones@redhat.com>
On 2017年07月24日 21:38, Andrew Jones wrote:
> Unregister the driver before removing multi-instance hotplug
> callbacks. This order avoids the warning issued from
> __cpuhp_remove_state_cpuslocked when the number of remaining
> instances isn't yet zero.
>
> Fixes: 8017c279196a ("net/virtio-net: Convert to hotplug state machine")
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> drivers/net/virtio_net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 99a26a9efec1..f41ab0ea942a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2743,9 +2743,9 @@ module_init(virtio_net_driver_init);
>
> static __exit void virtio_net_driver_exit(void)
> {
> + unregister_virtio_driver(&virtio_net_driver);
> cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
> cpuhp_remove_multi_state(virtionet_online);
> - unregister_virtio_driver(&virtio_net_driver);
> }
> module_exit(virtio_net_driver_exit);
>
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v12 5/8] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wei Wang @ 2017-07-26 3:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170723044036-mutt-send-email-mst@kernel.org>
On 07/23/2017 09:45 AM, Michael S. Tsirkin wrote:
> On Fri, Jul 14, 2017 at 03:12:43PM +0800, Wei Wang wrote:
>> On 07/14/2017 04:19 AM, Michael S. Tsirkin wrote:
>>> On Thu, Jul 13, 2017 at 03:42:35PM +0800, Wei Wang wrote:
>>>> On 07/12/2017 09:56 PM, Michael S. Tsirkin wrote:
>>>>> So the way I see it, there are several issues:
>>>>>
>>>>> - internal wait - forces multiple APIs like kick/kick_sync
>>>>> note how kick_sync can fail but your code never checks return code
>>>>> - need to re-write the last descriptor - might not work
>>>>> for alternative layouts which always expose descriptors
>>>>> immediately
>>>> Probably it wasn't clear. Please let me explain the two functions here:
>>>>
>>>> 1) virtqueue_add_chain_desc(vq, head_id, prev_id,..):
>>>> grabs a desc from the vq and inserts it to the chain tail (which is indexed
>>>> by
>>>> prev_id, probably better to call it tail_id). Then, the new added desc
>>>> becomes
>>>> the tail (i.e. the last desc). The _F_NEXT flag is cleared for each desc
>>>> when it's
>>>> added to the chain, and set when another desc comes to follow later.
>>> And this only works if there are multiple rings like
>>> avail + descriptor ring.
>>> It won't work e.g. with the proposed new layout where
>>> writing out a descriptor exposes it immediately.
>> I think it can support the 1.1 proposal, too. But before getting
>> into that, I think we first need to deep dive into the implementation
>> and usage of _first/next/last. The usage would need to lock the vq
>> from the first to the end (otherwise, the returned info about the number
>> of available desc in the vq, i.e. num_free, would be invalid):
>>
>> lock(vq);
>> add_first();
>> add_next();
>> add_last();
>> unlock(vq);
>>
>> However, I think the case isn't this simple, since we need to check more
>> things
>> after each add_xx() step. For example, if only one entry is available at the
>> time
>> we start to use the vq, that is, num_free is 0 after add_first(), we
>> wouldn't be
>> able to add_next and add_last. So, it would work like this:
>>
>> start:
>> ...get free page block..
>> lock(vq)
>> retry:
>> ret = add_first(..,&num_free,);
>> if(ret == -ENOSPC) {
>> goto retry;
>> } else if (!num_free) {
>> add_chain_head();
>> unlock(vq);
>> kick & wait;
>> goto start;
>> }
>> next_one:
>> ...get free page block..
>> add_next(..,&num_free,);
>> if (!num_free) {
>> add_chain_head();
>> unlock(vq);
>> kick & wait;
>> goto start;
>> } if (num_free == 1) {
>> ...get free page block..
>> add_last(..);
>> unlock(vq);
>> kick & wait;
>> goto start;
>> } else {
>> goto next_one;
>> }
>>
>> The above seems unnecessary to me to have three different APIs.
>> That's the reason to combine them into one virtqueue_add_chain_desc().
>>
>> -- or, do you have a different thought about using the three APIs?
>>
>>
>> Implementation Reference:
>>
>> struct desc_iterator {
>> unsigned int head;
>> unsigned int tail;
>> };
>>
>> add_first(*vq, *desc_iterator, *num_free, ..)
>> {
>> if (vq->vq.num_free < 1)
>> return -ENOSPC;
>> get_desc(&desc_id);
>> desc[desc_id].flag &= ~_F_NEXT;
>> desc_iterator->head = desc_id
>> desc_iterator->tail = desc_iterator->head;
>> *num_free = vq->vq.num_free;
>> }
>>
>> add_next(vq, desc_iterator, *num_free,..)
>> {
>> get_desc(&desc_id);
>> desc[desc_id].flag &= ~_F_NEXT;
>> desc[desc_iterator.tail].next = desc_id;
>> desc[desc_iterator->tail].flag |= _F_NEXT;
>> desc_iterator->tail = desc_id;
>> *num_free = vq->vq.num_free;
>> }
>>
>> add_last(vq, desc_iterator,..)
>> {
>> get_desc(&desc_id);
>> desc[desc_id].flag &= ~_F_NEXT;
>> desc[desc_iterator.tail].next = desc_id;
>> desc_iterator->tail = desc_id;
>>
>> add_chain_head(); // put the desc_iterator.head to the ring
>> }
>>
>>
>> Best,
>> Wei
> OK I thought this over. While we might need these new APIs in
> the future, I think that at the moment, there's a way to implement
> this feature that is significantly simpler. Just add each s/g
> as a separate input buffer.
Should it be an output buffer? I think output means from the
driver to device (i.e. DMA_TO_DEVICE).
>
> This needs zero new APIs.
>
> I know that follow-up patches need to add a header in front
> so you might be thinking: how am I going to add this
> header? The answer is quite simple - add it as a separate
> out header.
>
> Host will be able to distinguish between header and pages
> by looking at the direction, and - should we want to add
> IN data to header - additionally size (<4K => header).
I think this works fine when the cmdq is only used for
reporting the unused pages. It would be an issue
if there are other usages (e.g. report memory statistics)
interleaving. I think one solution would be to lock the cmdq until
a cmd usage is done ((e.g. all the unused pages are reported) ) -
in this case, the periodically updated guest memory statistics
may be delayed for a while occasionally when live migration starts.
Would this be acceptable? If not, probably we can have the cmdq
for one usage only.
Best,
Wei
^ permalink raw reply
* Re: [PATCH net-next] virtio-net: mark PM functions as __maybe_unused
From: Jason Wang @ 2017-07-26 3:21 UTC (permalink / raw)
To: Arnd Bergmann, Michael S. Tsirkin
Cc: Willem de Bruijn, netdev, Sebastian Andrzej Siewior,
John Fastabend, linux-kernel, virtualization, Eric Dumazet,
David S. Miller
In-Reply-To: <20170725153600.211694-1-arnd@arndb.de>
On 2017年07月25日 23:35, Arnd Bergmann wrote:
> After removing the reset function, the freeze and restore functions
> are now unused when CONFIG_PM_SLEEP is disabled:
>
> drivers/net/virtio_net.c:1881:12: error: 'virtnet_restore_up' defined but not used [-Werror=unused-function]
> static int virtnet_restore_up(struct virtio_device *vdev)
> drivers/net/virtio_net.c:1859:13: error: 'virtnet_freeze_down' defined but not used [-Werror=unused-function]
> static void virtnet_freeze_down(struct virtio_device *vdev)
>
> A more robust way to do this is to remove the #ifdef around the callers
> and instead mark them as __maybe_unused. The compiler will now just
> silently drop the unused code.
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/net/virtio_net.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d4751ce23b4f..1902701e15a9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2702,8 +2702,7 @@ static void virtnet_remove(struct virtio_device *vdev)
> free_netdev(vi->dev);
> }
>
> -#ifdef CONFIG_PM_SLEEP
> -static int virtnet_freeze(struct virtio_device *vdev)
> +static __maybe_unused int virtnet_freeze(struct virtio_device *vdev)
> {
> struct virtnet_info *vi = vdev->priv;
>
> @@ -2714,7 +2713,7 @@ static int virtnet_freeze(struct virtio_device *vdev)
> return 0;
> }
>
> -static int virtnet_restore(struct virtio_device *vdev)
> +static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
> {
> struct virtnet_info *vi = vdev->priv;
> int err;
> @@ -2730,7 +2729,6 @@ static int virtnet_restore(struct virtio_device *vdev)
>
> return 0;
> }
> -#endif
>
> static struct virtio_device_id id_table[] = {
> { VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
Acked-by: Jason Wang <jasowang@redhat.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next V2 5/5] virtio-net: switch off offloads on demand if possible on XDP set
From: Jason Wang @ 2017-07-26 3:19 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170725002700-mutt-send-email-mst@kernel.org>
On 2017年07月25日 05:29, Michael S. Tsirkin wrote:
> On Wed, Jul 19, 2017 at 04:54:49PM +0800, Jason Wang wrote:
>> Current XDP implementation wants guest offloads feature to be disabled
>> on device. This is inconvenient and means guest can't benefit from
>> offloads if XDP is not used. This patch tries to address this
>> limitation by disabling the offloads on demand through control guest
>> offloads. Guest offloads will be disabled and enabled on demand on XDP
>> set.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Doesn't look like my comments on v1 were addressed.
> Copying here:
>
> In fact, since we no longer reset when XDP is set,
> here device might have offloads enabled, buffers are
> used but not consumed, then XDP is set.
>
> This can result in
> - packet scattered across multiple buffers
> (handled correctly but need to update the comment)
We drop all gso packet now, so this won't happen.
> - packet may have VIRTIO_NET_HDR_F_NEEDS_CSUM, in that case
> the spec says "The checksum on the packet is incomplete".
> (probably needs to be handled by calculating the checksum).
This needs to be fixed, but it was not introduced by this commit but:
b00f70b0dacb virtio-net: unbreak csumed packets for XDP_PASS which
allows NEEDS_CSUM for mergeable buffer
bb91accf2733 virtio-net: XDP support for small buffers which forbid
checksumed packet for small buffer
Will post a patch to checksum the packet.
Thanks
>
> Jason, as David applied this patch already, can you comment
> on the issues please?
>
>> ---
>> drivers/net/virtio_net.c | 70 ++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 65 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index b3fc01d..5fbd15e 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -57,6 +57,11 @@ DECLARE_EWMA(pkt_len, 0, 64)
>>
>> #define VIRTNET_DRIVER_VERSION "1.0.0"
>>
>> +const unsigned long guest_offloads[] = { VIRTIO_NET_F_GUEST_TSO4,
>> + VIRTIO_NET_F_GUEST_TSO6,
>> + VIRTIO_NET_F_GUEST_ECN,
>> + VIRTIO_NET_F_GUEST_UFO };
>> +
>> struct virtnet_stats {
>> struct u64_stats_sync tx_syncp;
>> struct u64_stats_sync rx_syncp;
>> @@ -164,10 +169,13 @@ struct virtnet_info {
>> u8 ctrl_promisc;
>> u8 ctrl_allmulti;
>> u16 ctrl_vid;
>> + u64 ctrl_offloads;
>>
>> /* Ethtool settings */
>> u8 duplex;
>> u32 speed;
>> +
>> + unsigned long guest_offloads;
>> };
>>
>> struct padded_vnet_hdr {
>> @@ -1896,6 +1904,47 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>> return err;
>> }
>>
>> +static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>> +{
>> + struct scatterlist sg;
>> + vi->ctrl_offloads = cpu_to_virtio64(vi->vdev, offloads);
>> +
>> + sg_init_one(&sg, &vi->ctrl_offloads, sizeof(vi->ctrl_offloads));
>> +
>> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>> + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {
>> + dev_warn(&vi->dev->dev, "Fail to set guest offload. \n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int virtnet_clear_guest_offloads(struct virtnet_info *vi)
>> +{
>> + u64 offloads = 0;
>> +
>> + if (!vi->guest_offloads)
>> + return 0;
>> +
>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
>> + offloads = 1ULL << VIRTIO_NET_F_GUEST_CSUM;
>> +
>> + return virtnet_set_guest_offloads(vi, offloads);
>> +}
>> +
>> +static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
>> +{
>> + u64 offloads = vi->guest_offloads;
>> +
>> + if (!vi->guest_offloads)
>> + return 0;
>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
>> + offloads |= 1ULL << VIRTIO_NET_F_GUEST_CSUM;
>> +
>> + return virtnet_set_guest_offloads(vi, offloads);
>> +}
>> +
>> static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> struct netlink_ext_ack *extack)
>> {
>> @@ -1905,10 +1954,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> u16 xdp_qp = 0, curr_qp;
>> int i, err;
>>
>> - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
>> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
>> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO)) {
>> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
>> + && (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO))) {
>> NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing LRO, disable LRO first");
>> return -EOPNOTSUPP;
>> }
>> @@ -1955,6 +2005,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> for (i = 0; i < vi->max_queue_pairs; i++) {
>> old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>> rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>> + if (i == 0) {
>> + if (!old_prog)
>> + virtnet_clear_guest_offloads(vi);
>> + if (!prog)
>> + virtnet_restore_guest_offloads(vi);
>> + }
>> if (old_prog)
>> bpf_prog_put(old_prog);
>> virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
>> @@ -2588,6 +2644,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>> netif_carrier_on(dev);
>> }
>>
>> + for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
>> + if (virtio_has_feature(vi->vdev, guest_offloads[i]))
>> + set_bit(guest_offloads[i], &vi->guest_offloads);
>> +
>> pr_debug("virtnet: registered device %s with %d RX and TX vq's\n",
>> dev->name, max_queue_pairs);
>>
>> @@ -2684,7 +2744,7 @@ static struct virtio_device_id id_table[] = {
>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>> VIRTIO_NET_F_CTRL_MAC_ADDR, \
>> - VIRTIO_NET_F_MTU
>> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>
>> static unsigned int features[] = {
>> VIRTNET_FEATURES,
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Wei Wang @ 2017-07-26 2:22 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
amit.shah@redhat.com, kvm@vger.kernel.org, Michael S. Tsirkin,
linux-kernel@vger.kernel.org, liliang.opensource@gmail.com,
qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <20170725145333.GK26723@dhcp22.suse.cz>
On 07/25/2017 10:53 PM, Michal Hocko wrote:
> On Tue 25-07-17 14:47:16, Wang, Wei W wrote:
>> On Tuesday, July 25, 2017 8:42 PM, hal Hocko wrote:
>>> On Tue 25-07-17 19:56:24, Wei Wang wrote:
>>>> On 07/25/2017 07:25 PM, Michal Hocko wrote:
>>>>> On Tue 25-07-17 17:32:00, Wei Wang wrote:
>>>>>> On 07/24/2017 05:00 PM, Michal Hocko wrote:
>>>>>>> On Wed 19-07-17 20:01:18, Wei Wang wrote:
>>>>>>>> On 07/19/2017 04:13 PM, Michal Hocko wrote:
>>>>>>> [...
>>>> We don't need to do the pfn walk in the guest kernel. When the API
>>>> reports, for example, a 2MB free page block, the API caller offers to
>>>> the hypervisor the base address of the page block, and size=2MB, to
>>>> the hypervisor.
>>> So you want to skip pfn walks by regularly calling into the page allocator to
>>> update your bitmap. If that is the case then would an API that would allow you
>>> to update your bitmap via a callback be s sufficient? Something like
>>> void walk_free_mem(int node, int min_order,
>>> void (*visit)(unsigned long pfn, unsigned long nr_pages))
>>>
>>> The function will call the given callback for each free memory block on the given
>>> node starting from the given min_order. The callback will be strictly an atomic
>>> and very light context. You can update your bitmap from there.
>> I would need to introduce more about the background here:
>> The hypervisor and the guest live in their own address space. The hypervisor's bitmap
>> isn't seen by the guest. I think we also wouldn't be able to give a callback function
>> from the hypervisor to the guest in this case.
> How did you plan to use your original API which export struct page array
> then?
That's where the virtio-balloon driver comes in. It uses a shared ring
mechanism to
send the guest memory info to the hypervisor.
We didn't expose the struct page array from the guest to the hypervisor.
For example, when
a 2MB free page block is reported from the free page list, the info put
on the ring is just
(base address of the 2MB continuous memory, size=2M).
>
>>> This would address my main concern that the allocator internals would get
>>> outside of the allocator proper.
>> What issue would it have to expose the internal, for_each_zone()?
> zone is a MM internal concept. No code outside of the MM proper should
> really care about zones.
I think this is also what Andrew suggested in the previous discussion:
https://lkml.org/lkml/2017/3/16/951
Move the code to virtio-balloon and a little layering violation seems
acceptable.
Best,
Wei
^ permalink raw reply
* [PATCH net-next] virtio-net: mark PM functions as __maybe_unused
From: Arnd Bergmann @ 2017-07-25 15:35 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang
Cc: Willem de Bruijn, Arnd Bergmann, netdev,
Sebastian Andrzej Siewior, John Fastabend, linux-kernel,
virtualization, Eric Dumazet, David S. Miller
After removing the reset function, the freeze and restore functions
are now unused when CONFIG_PM_SLEEP is disabled:
drivers/net/virtio_net.c:1881:12: error: 'virtnet_restore_up' defined but not used [-Werror=unused-function]
static int virtnet_restore_up(struct virtio_device *vdev)
drivers/net/virtio_net.c:1859:13: error: 'virtnet_freeze_down' defined but not used [-Werror=unused-function]
static void virtnet_freeze_down(struct virtio_device *vdev)
A more robust way to do this is to remove the #ifdef around the callers
and instead mark them as __maybe_unused. The compiler will now just
silently drop the unused code.
Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/virtio_net.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d4751ce23b4f..1902701e15a9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2702,8 +2702,7 @@ static void virtnet_remove(struct virtio_device *vdev)
free_netdev(vi->dev);
}
-#ifdef CONFIG_PM_SLEEP
-static int virtnet_freeze(struct virtio_device *vdev)
+static __maybe_unused int virtnet_freeze(struct virtio_device *vdev)
{
struct virtnet_info *vi = vdev->priv;
@@ -2714,7 +2713,7 @@ static int virtnet_freeze(struct virtio_device *vdev)
return 0;
}
-static int virtnet_restore(struct virtio_device *vdev)
+static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
{
struct virtnet_info *vi = vdev->priv;
int err;
@@ -2730,7 +2729,6 @@ static int virtnet_restore(struct virtio_device *vdev)
return 0;
}
-#endif
static struct virtio_device_id id_table[] = {
{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
--
2.9.0
^ permalink raw reply related
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