[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] scp during migration with vhost fails
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] scp during migration with vhost fails |
Date: |
Tue, 26 Feb 2013 10:44:04 +0200 |
On Tue, Feb 26, 2013 at 02:41:03PM +0800, Jason Wang wrote:
> On 02/25/2013 06:01 PM, Michael S. Tsirkin wrote:
> > On Mon, Feb 25, 2013 at 02:11:44PM +0800, Jason Wang wrote:
> >> On 02/25/2013 01:57 PM, Jason Wang wrote:
> >>> On 02/24/2013 05:54 AM, Michael S. Tsirkin wrote:
> >>>> On Sat, Feb 23, 2013 at 10:49:29PM +0200, Michael S. Tsirkin wrote:
> >>>>> On Fri, Feb 22, 2013 at 11:33:53PM +0800, Jason Wang wrote:
> >>>>>> On 02/21/2013 07:23 PM, Michael S. Tsirkin wrote:
> >>>>>>> On Thu, Feb 21, 2013 at 05:57:04PM +0800, Jason Wang wrote:
> >>>>>>>> On 02/21/2013 12:48 AM, Michael S. Tsirkin wrote:
> >>>>>>>>> On Wed, Feb 20, 2013 at 04:23:52PM +0200, Michael S. Tsirkin wrote:
> >>>>>>>>>> On Fri, Feb 01, 2013 at 06:03:32PM +0800, Jason Wang wrote:
> >>>>>>>>>>> Hello all:
> >>>>>>>>>>>
> >>>>>>>>>>> During testing, I find doing scp during migration with vhost
> >>>>>>>>>>> fails with
> >>>>>>>>>>> warnings in guest like:
> >>>>>>>>>>>
> >>>>>>>>>>> Corrupted MAC on input.
> >>>>>>>>>>> Disconnecting: Packet corrupt.
> >>>>>>>>>>> lost connection
> >>>>>>>>>>>
> >>>>>>>>>>> Here's the bisect result:
> >>>>>>>>>>>
> >>>>>>>>>>> Commit a01672d3968cf91208666d371784110bfde9d4f8 kvm: convert to
> >>>>>>>>>>> MemoryListener
> >>>>>>>>>>> API is the last commit that works well.
> >>>>>>>>>>>
> >>>>>>>>>>> With commit 04097f7c5957273c578f72b9bd603ba6b1d69e33 vhost:
> >>>>>>>>>>> convert to
> >>>>>>>>>>> MemoryListener API, guest network is unusable with warning of
> >>>>>>>>>>> "bad gso type"
> >>>>>>>>>>>
> >>>>>>>>>>> With commit d743c382861eaa1e13f503b05aba5a382a7e7f7c vhost: fix
> >>>>>>>>>>> incorrect
> >>>>>>>>>>> userspace address, guest network is available, but scp during
> >>>>>>>>>>> migration may
> >>>>>>>>>>> fail.
> >>>>>>>>>>>
> >>>>>>>>>>> Looks like the issue is related to memory api, any thoughts?
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks
> >>>>>>>>>> Tried to reproduce this for a while without success.
> >>>>>>>>>> Which command line was used?
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> MST
> >>>>>>>>> Could be we are not syncing all that we should?
> >>>>>>>>> Does the following hack make the problem go away?
> >>>>>>>>>
> >>>>>>>>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>>>>>>>> index 8d41fdb..a7a0412 100644
> >>>>>>>>> --- a/hw/vhost.c
> >>>>>>>>> +++ b/hw/vhost.c
> >>>>>>>>> @@ -69,6 +69,8 @@ static int vhost_sync_dirty_bitmap(struct
> >>>>>>>>> vhost_dev *dev,
> >>>>>>>>> hwaddr end_addr)
> >>>>>>>>> {
> >>>>>>>>> int i;
> >>>>>>>>> + start_addr = 0x0;
> >>>>>>>>> + end_addr = ~0x0ull;
> >>>>>>>>>
> >>>>>>>>> if (!dev->log_enabled || !dev->started) {
> >>>>>>>>> return 0;
> >>>>>>>>>
> >>>>>>>> Still can reproduce with this. From the bisect result, the vhost
> >>>>>>>> dirty
> >>>>>>>> bitmap sync itself looks ok but something wrong when converting to
> >>>>>>>> memory listener.
> >>>>>>> Reading the code carefully, I found two bugs introduced during
> >>>>>>> this conversion. Patch below, could you please try?
> >>>>>>>
> >>>>>>> vhost: memory sync fixes
> >>>>>>>
> >>>>>>> This fixes two bugs related to memory sync during
> >>>>>>> migration:
> >>>>>>> - ram address calculation was missing the chunk
> >>>>>>> address, so the wrong page was dirtied
> >>>>>>> - one after last was used instead of the
> >>>>>>> end address of a region, which might overflow to 0
> >>>>>>> and cause us to skip the region when the region ends at
> >>>>>>> ~0x0ull.
> >>>>>>>
> >>>>>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
> >>>>>>>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>>>>>> index 8d41fdb..dbf6b46 100644
> >>>>>>> --- a/hw/vhost.c
> >>>>>>> +++ b/hw/vhost.c
> >>>>>>> @@ -55,7 +55,7 @@ static void vhost_dev_sync_region(struct vhost_dev
> >>>>>>> *dev,
> >>>>>>> ffsll(log) : ffs(log))) {
> >>>>>>> ram_addr_t ram_addr;
> >>>>>>> bit -= 1;
> >>>>>>> - ram_addr = section->offset_within_region + bit *
> >>>>>>> VHOST_LOG_PAGE;
> >>>>>>> + ram_addr = section->offset_within_region + addr + bit *
> >>>>>>> VHOST_LOG_PAGE;
> >>>>>>> memory_region_set_dirty(section->mr, ram_addr,
> >>>>>>> VHOST_LOG_PAGE);
> >>>>>>> log &= ~(0x1ull << bit);
> >>>>>>> }
> >>>>>>> @@ -94,7 +94,7 @@ static void vhost_log_sync(MemoryListener *listener,
> >>>>>>> struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> >>>>>>> memory_listener);
> >>>>>>> hwaddr start_addr = section->offset_within_address_space;
> >>>>>>> - hwaddr end_addr = start_addr + section->size;
> >>>>>>> + hwaddr end_addr = start_addr + section->size - 1;
> >>>>>>>
> >>>>>>> vhost_sync_dirty_bitmap(dev, section, start_addr, end_addr);
> >>>>>>> }
> >>>>>>>
> >>>>>> I can still reproduce the issue with this patch.
> >>>>> Yes it's still wrong. We need the following on top.
> >>>>> Could you try please?
> >>>>>
> >>>>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>>>> index dbf6b46..c324903 100644
> >>>>> --- a/hw/vhost.c
> >>>>> +++ b/hw/vhost.c
> >>>>> @@ -29,7 +29,7 @@ static void vhost_dev_sync_region(struct vhost_dev
> >>>>> *dev,
> >>>>> uint64_t end = MIN(mlast, rlast);
> >>>>> vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
> >>>>> vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
> >>>>> - uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
> >>>>> + uint64_t addr = 0;
> >>>>>
> >>>>> if (end < start) {
> >>>>> return;
> >>>> Sorry, scratch that last one, sorry.
> >>>> This should be the right thing, I think: on top of
> >>>> 'vhost: memory sync fixes'.
> >>>>
> >>>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>>> index dbf6b46..72c0095 100644
> >>>> --- a/hw/vhost.c
> >>>> +++ b/hw/vhost.c
> >>>> @@ -53,9 +53,10 @@ static void vhost_dev_sync_region(struct vhost_dev
> >>>> *dev,
> >>>> log = __sync_fetch_and_and(from, 0);
> >>>> while ((bit = sizeof(log) > sizeof(int) ?
> >>>> ffsll(log) : ffs(log))) {
> >>>> - ram_addr_t ram_addr;
> >>>> + hwaddr ram_addr;
> >>>> bit -= 1;
> >>>> - ram_addr = section->offset_within_region + addr + bit *
> >>>> VHOST_LOG_PAGE;
> >>>> + ram_addr = addr + bit * VHOST_LOG_PAGE -
> >>>> + section->mr->offset_within_address_space;
> >>> should be section->offset_within_address_space
> >>>> memory_region_set_dirty(section->mr, ram_addr,
> >>>> VHOST_LOG_PAGE);
> >>>> log &= ~(0x1ull << bit);
> >>>> }
> >>>>
> >>>>
> >>> Still can reproduce. An interesting thing is after I chage the
> >>> section->offset_within_address_space to section->mr->ram_addr[1]. I
> >>> can't reproduce the issue. I haven't read all the codes, but it looks
> >>> like something is wrong with the valueof
> >>> section->offset_within_address_space? Thanks
> >> It's ok since we need offset inside the region as the second parameter
> >> of memory_region_set_dirty().
> >>> [1]
> >>> diff --git a/hw/vhost.c b/hw/vhost.c
> >>> index 8d41fdb..785e68e 100644
> >>> --- a/hw/vhost.c
> >>> +++ b/hw/vhost.c
> >>> @@ -55,7 +55,7 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
> >>> ffsll(log) : ffs(log))) {
> >>> ram_addr_t ram_addr;
> >>> bit -= 1;
> >>> - ram_addr = section->offset_within_region + bit *
> >>> VHOST_LOG_PAGE;
> >>> + ram_addr = addr + bit * VHOST_LOG_PAGE -
> >>> section->mr->ram_addr;
> >>> memory_region_set_dirty(section->mr, ram_addr,
> >>> VHOST_LOG_PAGE);
> >>> log &= ~(0x1ull << bit);
> >>> }
> >>>
> >>>
> >
> > OK I think now I get it.
> > The following should be the real thing.
> >
> >
> > diff --git a/hw/vhost.c b/hw/vhost.c
> > index dbf6b46..2d8ead8 100644
> > --- a/hw/vhost.c
> > +++ b/hw/vhost.c
> > @@ -53,10 +53,14 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
> > log = __sync_fetch_and_and(from, 0);
> > while ((bit = sizeof(log) > sizeof(int) ?
> > ffsll(log) : ffs(log))) {
> > - ram_addr_t ram_addr;
> > + hwaddr page_addr;
> > + hwaddr section_offset;
> > + hwaddr mr_offset;
> > bit -= 1;
> > - ram_addr = section->offset_within_region + addr + bit *
> > VHOST_LOG_PAGE;
> > - memory_region_set_dirty(section->mr, ram_addr, VHOST_LOG_PAGE);
> > + page_addr = addr + bit * VHOST_LOG_PAGE;
> > + section_offset = ram_addr -
> > section->offset_within_address_space;
>
> Should be page_addr here.
> > + mr_offset = section_offset + section->offset_within_region;
> > + memory_region_set_dirty(section->mr, mr_offset,
> > VHOST_LOG_PAGE);
> > log &= ~(0x1ull << bit);
> > }
> > addr += VHOST_LOG_CHUNK;
> >
> >
> >
> I get segfault with this patch, and looks like we need to check whether
> the regions are overlapped in the begining of vhost_dev_sync_region:
>
> if (!ranges_overlap(start, end - start + 1,
> section->offset_within_address_space,
> section->size)) {
> return;
> }
>
> With this, no segfault and I can not reproduce the issue again.
OK, and I guess this works because sync sections ATM do not cross
tracking sections, but it does not seem to be required by the API, so I
think we really should be more careful and only sync the correct range.
Signed-off-by: Michael S. Tsirkin <address@hidden>
diff --git a/hw/vhost.c b/hw/vhost.c
index dbf6b46..37777c2 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -53,10 +53,14 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
log = __sync_fetch_and_and(from, 0);
while ((bit = sizeof(log) > sizeof(int) ?
ffsll(log) : ffs(log))) {
- ram_addr_t ram_addr;
+ hwaddr page_addr;
+ hwaddr section_offset;
+ hwaddr mr_offset;
bit -= 1;
- ram_addr = section->offset_within_region + addr + bit *
VHOST_LOG_PAGE;
- memory_region_set_dirty(section->mr, ram_addr, VHOST_LOG_PAGE);
+ page_addr = addr + bit * VHOST_LOG_PAGE;
+ section_offset = page_addr - section->offset_within_address_space;
+ mr_offset = section_offset + section->offset_within_region;
+ memory_region_set_dirty(section->mr, mr_offset, VHOST_LOG_PAGE);
log &= ~(0x1ull << bit);
}
addr += VHOST_LOG_CHUNK;
@@ -65,14 +69,21 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
MemoryRegionSection *section,
- hwaddr start_addr,
- hwaddr end_addr)
+ hwaddr first,
+ hwaddr last)
{
int i;
+ hwaddr start_addr;
+ hwaddr end_addr;
if (!dev->log_enabled || !dev->started) {
return 0;
}
+ start_addr = section->offset_within_address_space;
+ end_addr = range_get_last(start_addr, section->size);
+ start_addr = MAX(first, start_addr);
+ end_addr = MIN(last, end_addr);
+
for (i = 0; i < dev->mem->nregions; ++i) {
struct vhost_memory_region *reg = dev->mem->regions + i;
vhost_dev_sync_region(dev, section, start_addr, end_addr,
@@ -93,10 +104,18 @@ static void vhost_log_sync(MemoryListener *listener,
{
struct vhost_dev *dev = container_of(listener, struct vhost_dev,
memory_listener);
- hwaddr start_addr = section->offset_within_address_space;
- hwaddr end_addr = start_addr + section->size - 1;
+ vhost_sync_dirty_bitmap(dev, section, 0x0, ~0x0ULL);
+}
- vhost_sync_dirty_bitmap(dev, section, start_addr, end_addr);
+static void vhost_log_sync_range(struct vhost_dev *dev,
+ hwaddr first, hwaddr last)
+{
+ int i;
+ /* FIXME: this is N^2 in number of sections */
+ for (i = 0; i < dev->n_mem_sections; ++i) {
+ MemoryRegionSection *section = &dev->mem_sections[i];
+ vhost_sync_dirty_bitmap(dev, section, first, last);
+ }
}
/* Assign/unassign. Keep an unsorted array of non-overlapping
@@ -268,16 +287,15 @@ static inline void vhost_dev_log_resize(struct vhost_dev*
dev, uint64_t size)
{
vhost_log_chunk_t *log;
uint64_t log_base;
- int r, i;
+ int r;
log = g_malloc0(size * sizeof *log);
log_base = (uint64_t)(unsigned long)log;
r = ioctl(dev->control, VHOST_SET_LOG_BASE, &log_base);
assert(r >= 0);
- for (i = 0; i < dev->n_mem_sections; ++i) {
- /* Sync only the range covered by the old log */
- vhost_sync_dirty_bitmap(dev, &dev->mem_sections[i], 0,
- dev->log_size * VHOST_LOG_CHUNK - 1);
+ /* Sync only the range covered by the old log */
+ if (dev->log_size) {
+ vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1);
}
if (dev->log) {
g_free(dev->log);
@@ -1014,10 +1032,7 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice
*vdev)
hdev->vqs + i,
hdev->vq_index + i);
}
- for (i = 0; i < hdev->n_mem_sections; ++i) {
- vhost_sync_dirty_bitmap(hdev, &hdev->mem_sections[i],
- 0, (hwaddr)~0x0ull);
- }
+ vhost_log_sync_range(hdev, 0, ~0x0ull);
hdev->started = false;
g_free(hdev->log);
- Re: [Qemu-devel] scp during migration with vhost fails, (continued)
- Re: [Qemu-devel] scp during migration with vhost fails, Michael S. Tsirkin, 2013/02/20
- Re: [Qemu-devel] scp during migration with vhost fails, Jason Wang, 2013/02/21
- Re: [Qemu-devel] scp during migration with vhost fails, Michael S. Tsirkin, 2013/02/21
- Re: [Qemu-devel] scp during migration with vhost fails, Jason Wang, 2013/02/22
- Re: [Qemu-devel] scp during migration with vhost fails, Michael S. Tsirkin, 2013/02/23
- Re: [Qemu-devel] scp during migration with vhost fails, Michael S. Tsirkin, 2013/02/23
- Re: [Qemu-devel] scp during migration with vhost fails, Jason Wang, 2013/02/25
- Re: [Qemu-devel] scp during migration with vhost fails, Jason Wang, 2013/02/25
- Re: [Qemu-devel] scp during migration with vhost fails, Michael S. Tsirkin, 2013/02/25
- Re: [Qemu-devel] scp during migration with vhost fails, Jason Wang, 2013/02/26
- Re: [Qemu-devel] scp during migration with vhost fails,
Michael S. Tsirkin <=
- Re: [Qemu-devel] scp during migration with vhost fails, Jason Wang, 2013/02/26
- Re: [Qemu-devel] scp during migration with vhost fails, Michael S. Tsirkin, 2013/02/25
Re: [Qemu-devel] scp during migration with vhost fails, Jason Wang, 2013/02/21