| Message ID | 20180116180408.11279-1-dgilbert@redhat.com |
|---|---|
| Headers | show |
| Series | Rework vhost memory region updates | expand |
On Tue, Jan 16, 2018 at 06:04:01PM +0000, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Hi, > This patch set reworks the way the vhost code handles changes in > physical address space layout that came from a discussion with Igor. > > Its intention is to simplify a lot of the update code, > and to make it easier for the postcopy+shared code to > do the hugepage alignments that are needed. > > Instead of inserting/removing each section during the add/del > callbacks of the listener, we start afresh and build a list > from the add and nop callbacks, then at the end compare the list > we've built with the exisiting list. > > v6 > Tidy ups from Igor > The biggest change is moving the 'Move log_dirty check' to be > the last patch in the set. > > Dr. David Alan Gilbert (7): > vhost: Build temporary section list and deref after commit > vhost: Simplify ring verification checks > vhost: Merge sections added to temporary list > vhost: Regenerate region list from changed sections list > vhost: Clean out old vhost_set_memory and friends > vhost: Merge and delete unused callbacks > vhost: Move log_dirty check > > hw/virtio/trace-events | 6 + > hw/virtio/vhost.c | 497 ++++++++++++++++------------------------------ > include/hw/virtio/vhost.h | 5 +- > 3 files changed, 180 insertions(+), 328 deletions(-) Seems to trigger errors with clang runtime sanitizer: /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:64:33: note: nonnull attribute specified here > -- > 2.14.3
* Michael S. Tsirkin (mst@redhat.com) wrote: > On Tue, Jan 16, 2018 at 06:04:01PM +0000, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Hi, > > This patch set reworks the way the vhost code handles changes in > > physical address space layout that came from a discussion with Igor. > > > > Its intention is to simplify a lot of the update code, > > and to make it easier for the postcopy+shared code to > > do the hugepage alignments that are needed. > > > > Instead of inserting/removing each section during the add/del > > callbacks of the listener, we start afresh and build a list > > from the add and nop callbacks, then at the end compare the list > > we've built with the exisiting list. > > > > v6 > > Tidy ups from Igor > > The biggest change is moving the 'Move log_dirty check' to be > > the last patch in the set. > > > > Dr. David Alan Gilbert (7): > > vhost: Build temporary section list and deref after commit > > vhost: Simplify ring verification checks > > vhost: Merge sections added to temporary list > > vhost: Regenerate region list from changed sections list > > vhost: Clean out old vhost_set_memory and friends > > vhost: Merge and delete unused callbacks > > vhost: Move log_dirty check > > > > hw/virtio/trace-events | 6 + > > hw/virtio/vhost.c | 497 ++++++++++++++++------------------------------ > > include/hw/virtio/vhost.h | 5 +- > > 3 files changed, 180 insertions(+), 328 deletions(-) > > > Seems to trigger errors with clang runtime sanitizer: > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > /usr/include/string.h:64:33: note: nonnull attribute specified here > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > /usr/include/string.h:64:33: note: nonnull attribute specified here > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > /usr/include/string.h:64:33: note: nonnull attribute specified here > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > /usr/include/string.h:64:33: note: nonnull attribute specified here > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > /usr/include/string.h:64:33: note: nonnull attribute specified here > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > /usr/include/string.h:64:33: note: nonnull attribute specified here > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > /usr/include/string.h:64:33: note: nonnull attribute specified here > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > /usr/include/string.h:64:33: note: nonnull attribute specified here How are you running that test? Can you add this printf and tell me what it's seeing? /* Same size, lets check the contents */ fprintf(stderr, "%s: %p %p %d\n", __func__, dev->mem_sections, old_sections, n_old_sections); changed = memcmp(dev->mem_sections, old_sections, n_old_sections * sizeof(old_sections[0])) != 0; I'm seeing a bunch of calls where both pointers are NULL, but n_old_sections is 0, which feels legal to me. I guess we could make it: changed = n_old_sections ? memcmp(....) : false; just to shut clang up. Dave > > > > -- > > 2.14.3 -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Jan 18, 2018 at 07:59:35PM +0000, Dr. David Alan Gilbert wrote: > * Michael S. Tsirkin (mst@redhat.com) wrote: > > On Tue, Jan 16, 2018 at 06:04:01PM +0000, Dr. David Alan Gilbert (git) wrote: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > Hi, > > > This patch set reworks the way the vhost code handles changes in > > > physical address space layout that came from a discussion with Igor. > > > > > > Its intention is to simplify a lot of the update code, > > > and to make it easier for the postcopy+shared code to > > > do the hugepage alignments that are needed. > > > > > > Instead of inserting/removing each section during the add/del > > > callbacks of the listener, we start afresh and build a list > > > from the add and nop callbacks, then at the end compare the list > > > we've built with the exisiting list. > > > > > > v6 > > > Tidy ups from Igor > > > The biggest change is moving the 'Move log_dirty check' to be > > > the last patch in the set. > > > > > > Dr. David Alan Gilbert (7): > > > vhost: Build temporary section list and deref after commit > > > vhost: Simplify ring verification checks > > > vhost: Merge sections added to temporary list > > > vhost: Regenerate region list from changed sections list > > > vhost: Clean out old vhost_set_memory and friends > > > vhost: Merge and delete unused callbacks > > > vhost: Move log_dirty check > > > > > > hw/virtio/trace-events | 6 + > > > hw/virtio/vhost.c | 497 ++++++++++++++++------------------------------ > > > include/hw/virtio/vhost.h | 5 +- > > > 3 files changed, 180 insertions(+), 328 deletions(-) > > > > > > Seems to trigger errors with clang runtime sanitizer: > > > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > How are you running that test? One Fedora: dnf install clang '/scm/qemu-clang/../qemu/configure' '--cc=clang' '--cxx=clang++' '--extra-cflags=-fsanitize=undefined -ferror-limit=10000 -fno-sanitize=shift-base' make make check. > Can you add this printf and tell me what > it's seeing? > > /* Same size, lets check the contents */ > fprintf(stderr, "%s: %p %p %d\n", __func__, dev->mem_sections, old_sections, n_old_sections); > changed = memcmp(dev->mem_sections, old_sections, > n_old_sections * sizeof(old_sections[0])) != 0; > > I'm seeing a bunch of calls where both pointers are NULL, but > n_old_sections is 0, which feels legal to me. https://stackoverflow.com/questions/16362925/can-i-pass-a-null-pointer-to-memcmp says it's invalid. > I guess we could make it: > changed = n_old_sections ? memcmp(....) : false; > just to shut clang up. > > Dave > > > > > > > > -- > > > 2.14.3 > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Michael S. Tsirkin (mst@redhat.com) wrote: > On Thu, Jan 18, 2018 at 07:59:35PM +0000, Dr. David Alan Gilbert wrote: > > * Michael S. Tsirkin (mst@redhat.com) wrote: > > > On Tue, Jan 16, 2018 at 06:04:01PM +0000, Dr. David Alan Gilbert (git) wrote: > > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > > > > > Hi, > > > > This patch set reworks the way the vhost code handles changes in > > > > physical address space layout that came from a discussion with Igor. > > > > > > > > Its intention is to simplify a lot of the update code, > > > > and to make it easier for the postcopy+shared code to > > > > do the hugepage alignments that are needed. > > > > > > > > Instead of inserting/removing each section during the add/del > > > > callbacks of the listener, we start afresh and build a list > > > > from the add and nop callbacks, then at the end compare the list > > > > we've built with the exisiting list. > > > > > > > > v6 > > > > Tidy ups from Igor > > > > The biggest change is moving the 'Move log_dirty check' to be > > > > the last patch in the set. > > > > > > > > Dr. David Alan Gilbert (7): > > > > vhost: Build temporary section list and deref after commit > > > > vhost: Simplify ring verification checks > > > > vhost: Merge sections added to temporary list > > > > vhost: Regenerate region list from changed sections list > > > > vhost: Clean out old vhost_set_memory and friends > > > > vhost: Merge and delete unused callbacks > > > > vhost: Move log_dirty check > > > > > > > > hw/virtio/trace-events | 6 + > > > > hw/virtio/vhost.c | 497 ++++++++++++++++------------------------------ > > > > include/hw/virtio/vhost.h | 5 +- > > > > 3 files changed, 180 insertions(+), 328 deletions(-) > > > > > > > > > Seems to trigger errors with clang runtime sanitizer: > > > > > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > /scm/qemu/hw/virtio/vhost.c:425:26: runtime error: null pointer passed as argument 1, which is declared to never be null > > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > /scm/qemu/hw/virtio/vhost.c:425:45: runtime error: null pointer passed as argument 2, which is declared to never be null > > > /usr/include/string.h:64:33: note: nonnull attribute specified here > > > > > > How are you running that test? > > One Fedora: > dnf install clang > > '/scm/qemu-clang/../qemu/configure' '--cc=clang' '--cxx=clang++' '--extra-cflags=-fsanitize=undefined -ferror-limit=10000 -fno-sanitize=shift-base' > make > make check. Thanks. > > Can you add this printf and tell me what > > it's seeing? > > > > /* Same size, lets check the contents */ > > fprintf(stderr, "%s: %p %p %d\n", __func__, dev->mem_sections, old_sections, n_old_sections); > > changed = memcmp(dev->mem_sections, old_sections, > > n_old_sections * sizeof(old_sections[0])) != 0; > > > > I'm seeing a bunch of calls where both pointers are NULL, but > > n_old_sections is 0, which feels legal to me. > > https://stackoverflow.com/questions/16362925/can-i-pass-a-null-pointer-to-memcmp > > says it's invalid. Posted v7 with trivial fix for it. (It seems rather ungenerous of the standard to be fussy about that). Dave > > I guess we could make it: > > changed = n_old_sections ? memcmp(....) : false; > > just to shut clang up. > > > > Dave > > > > > > > > > > > > -- > > > > 2.14.3 > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Hi, This patch set reworks the way the vhost code handles changes in physical address space layout that came from a discussion with Igor. Its intention is to simplify a lot of the update code, and to make it easier for the postcopy+shared code to do the hugepage alignments that are needed. Instead of inserting/removing each section during the add/del callbacks of the listener, we start afresh and build a list from the add and nop callbacks, then at the end compare the list we've built with the exisiting list. v6 Tidy ups from Igor The biggest change is moving the 'Move log_dirty check' to be the last patch in the set. Dr. David Alan Gilbert (7): vhost: Build temporary section list and deref after commit vhost: Simplify ring verification checks vhost: Merge sections added to temporary list vhost: Regenerate region list from changed sections list vhost: Clean out old vhost_set_memory and friends vhost: Merge and delete unused callbacks vhost: Move log_dirty check hw/virtio/trace-events | 6 + hw/virtio/vhost.c | 497 ++++++++++++++++------------------------------ include/hw/virtio/vhost.h | 5 +- 3 files changed, 180 insertions(+), 328 deletions(-)