Message ID | 512AFD62.9000106@redhat.com |
---|---|
State | New |
Headers | show |
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 <mst@redhat.com> >>>>> >>>>> --- >>>>> >>>>> 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); > } > >
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);