include/exec/memory.h | 15 +++++++++++++-- include/exec/ram_addr.h | 4 ++-- exec.c | 2 +- hw/block/nvme.c | 6 ++---- memory.c | 12 +++++++++--- target/arm/helper.c | 2 +- hw/block/Makefile.objs | 2 +- 7 files changed, 29 insertions(+), 14 deletions(-)
Let the NVMe emulated device be target-agnostic. It is not clear if dccvap_writefn() really needs memory_region_writeback() or could use memory_region_msync(). Philippe Mathieu-Daudé (4): memory: Rename memory_region_do_writeback -> memory_region_writeback memory: Extract memory_region_msync() from memory_region_writeback() hw/block: Let the NVMe emulated device be target-agnostic exec: Rename qemu_ram_writeback() as qemu_ram_msync() include/exec/memory.h | 15 +++++++++++++-- include/exec/ram_addr.h | 4 ++-- exec.c | 2 +- hw/block/nvme.c | 6 ++---- memory.c | 12 +++++++++--- target/arm/helper.c | 2 +- hw/block/Makefile.objs | 2 +- 7 files changed, 29 insertions(+), 14 deletions(-) -- 2.21.3
On Fri, May 08, 2020 at 08:24:52AM +0200, Philippe Mathieu-Daudé wrote: > Let the NVMe emulated device be target-agnostic. > > It is not clear if dccvap_writefn() really needs > memory_region_writeback() or could use memory_region_msync(). > > Philippe Mathieu-Daudé (4): > memory: Rename memory_region_do_writeback -> memory_region_writeback > memory: Extract memory_region_msync() from memory_region_writeback() > hw/block: Let the NVMe emulated device be target-agnostic > exec: Rename qemu_ram_writeback() as qemu_ram_msync() > > include/exec/memory.h | 15 +++++++++++++-- > include/exec/ram_addr.h | 4 ++-- > exec.c | 2 +- > hw/block/nvme.c | 6 ++---- > memory.c | 12 +++++++++--- > target/arm/helper.c | 2 +- > hw/block/Makefile.objs | 2 +- > 7 files changed, 29 insertions(+), 14 deletions(-) > > -- > 2.21.3 > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 21/05/20 14:32, Stefan Hajnoczi wrote: > On Fri, May 08, 2020 at 08:24:52AM +0200, Philippe Mathieu-Daudé wrote: >> Let the NVMe emulated device be target-agnostic. >> >> It is not clear if dccvap_writefn() really needs >> memory_region_writeback() or could use memory_region_msync(). >> >> Philippe Mathieu-Daudé (4): >> memory: Rename memory_region_do_writeback -> memory_region_writeback >> memory: Extract memory_region_msync() from memory_region_writeback() >> hw/block: Let the NVMe emulated device be target-agnostic >> exec: Rename qemu_ram_writeback() as qemu_ram_msync() >> >> include/exec/memory.h | 15 +++++++++++++-- >> include/exec/ram_addr.h | 4 ++-- >> exec.c | 2 +- >> hw/block/nvme.c | 6 ++---- >> memory.c | 12 +++++++++--- >> target/arm/helper.c | 2 +- >> hw/block/Makefile.objs | 2 +- >> 7 files changed, 29 insertions(+), 14 deletions(-) >> >> -- >> 2.21.3 >> >> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
On 5/21/20 2:46 PM, Paolo Bonzini wrote: > On 21/05/20 14:32, Stefan Hajnoczi wrote: >> On Fri, May 08, 2020 at 08:24:52AM +0200, Philippe Mathieu-Daudé wrote: >>> Let the NVMe emulated device be target-agnostic. >>> >>> It is not clear if dccvap_writefn() really needs >>> memory_region_writeback() or could use memory_region_msync(). >>> >>> Philippe Mathieu-Daudé (4): >>> memory: Rename memory_region_do_writeback -> memory_region_writeback >>> memory: Extract memory_region_msync() from memory_region_writeback() >>> hw/block: Let the NVMe emulated device be target-agnostic >>> exec: Rename qemu_ram_writeback() as qemu_ram_msync() >>> >>> include/exec/memory.h | 15 +++++++++++++-- >>> include/exec/ram_addr.h | 4 ++-- >>> exec.c | 2 +- >>> hw/block/nvme.c | 6 ++---- >>> memory.c | 12 +++++++++--- >>> target/arm/helper.c | 2 +- >>> hw/block/Makefile.objs | 2 +- >>> 7 files changed, 29 insertions(+), 14 deletions(-) >>> >>> -- >>> 2.21.3 >>> >>> >> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> > > Acked-by: Paolo Bonzini <pbonzini@redhat.com> > Thanks both. Stefan, do you mind taking the series via your block tree? Thanks, Phil.
On 08/05/20 08:24, Philippe Mathieu-Daudé wrote: > It is not clear if dccvap_writefn() really needs > memory_region_writeback() or could use memory_region_msync(). Indeed, I don't understand the code and why it matters that mr->dirty_log_mask is nonzero. mr->dirty_log_mask tells if dirty tracking has been enabled, not if the page is dirty. It would always be true during live migration and when running on TCG, but otherwise it would always be false. Beata, can you explain what you had in mind? :) Paolo
On Fri, 8 May 2020 at 07:33, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 08/05/20 08:24, Philippe Mathieu-Daudé wrote: > > It is not clear if dccvap_writefn() really needs > > memory_region_writeback() or could use memory_region_msync(). > > Indeed, I don't understand the code and why it matters that > mr->dirty_log_mask is nonzero. > > mr->dirty_log_mask tells if dirty tracking has been enabled, not if the > page is dirty. It would always be true during live migration and when > running on TCG, but otherwise it would always be false. > > Beata, can you explain what you had in mind? :) > It has been a while ... , but the intention there was to skip the sync if there is nothing to be synced in the first place - so for performance reasons. I honestly do not recall why I went for the dirty_log_mask, as that seems not to be the right choice . BR Beata > Paolo >
On 08/05/20 17:20, Beata Michalska wrote: >> >> mr->dirty_log_mask tells if dirty tracking has been enabled, not if the >> page is dirty. It would always be true during live migration and when >> running on TCG, but otherwise it would always be false. >> >> Beata, can you explain what you had in mind? :) >> > It has been a while ... , but the intention there was to skip the sync > if there is nothing to be synced in the first place - so for performance > reasons. I honestly do not recall why I went for the dirty_log_mask, > as that seems not to be the right choice . You probably wanted to look at the dirty bitmap, but you would have to define a new bitmap rather than looking at dirty_log_mask. But that's cool, because it means that we can just remove it! Thanks, Paolo
On 5/8/20 5:33 PM, Paolo Bonzini wrote: > On 08/05/20 17:20, Beata Michalska wrote: >>> >>> mr->dirty_log_mask tells if dirty tracking has been enabled, not if the >>> page is dirty. It would always be true during live migration and when >>> running on TCG, but otherwise it would always be false. >>> >>> Beata, can you explain what you had in mind? :) >>> >> It has been a while ... , but the intention there was to skip the sync >> if there is nothing to be synced in the first place - so for performance >> reasons. I honestly do not recall why I went for the dirty_log_mask, >> as that seems not to be the right choice . > > You probably wanted to look at the dirty bitmap, but you would have to > define a new bitmap rather than looking at dirty_log_mask. > > But that's cool, because it means that we can just remove it! Thanks, So I understand I can simply rename memory_region_writeback -> memory_region_msync in patch #2.
On Fri, May 8, 2020 at 5:34 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 5/8/20 5:33 PM, Paolo Bonzini wrote: > > On 08/05/20 17:20, Beata Michalska wrote: > >>> > >>> mr->dirty_log_mask tells if dirty tracking has been enabled, not if the > >>> page is dirty. It would always be true during live migration and when > >>> running on TCG, but otherwise it would always be false. > >>> > >>> Beata, can you explain what you had in mind? :) > >>> > >> It has been a while ... , but the intention there was to skip the sync > >> if there is nothing to be synced in the first place - so for performance > >> reasons. I honestly do not recall why I went for the dirty_log_mask, > >> as that seems not to be the right choice . > > > > You probably wanted to look at the dirty bitmap, but you would have to > > define a new bitmap rather than looking at dirty_log_mask. > > > > But that's cool, because it means that we can just remove it! Thanks, > > So I understand I can simply rename memory_region_writeback -> > memory_region_msync in patch #2. (removing the mr->dirty_log_mask).
On 08/05/20 17:35, Philippe Mathieu-Daudé wrote: >> So I understand I can simply rename memory_region_writeback -> >> memory_region_msync in patch #2. > > (removing the mr->dirty_log_mask). > Yes. Paolo
On Fri, May 08, 2020 at 08:24:52AM +0200, Philippe Mathieu-Daudé wrote: > Let the NVMe emulated device be target-agnostic. > > It is not clear if dccvap_writefn() really needs > memory_region_writeback() or could use memory_region_msync(). > > Philippe Mathieu-Daudé (4): > memory: Rename memory_region_do_writeback -> memory_region_writeback > memory: Extract memory_region_msync() from memory_region_writeback() > hw/block: Let the NVMe emulated device be target-agnostic > exec: Rename qemu_ram_writeback() as qemu_ram_msync() > > include/exec/memory.h | 15 +++++++++++++-- > include/exec/ram_addr.h | 4 ++-- > exec.c | 2 +- > hw/block/nvme.c | 6 ++---- > memory.c | 12 +++++++++--- > target/arm/helper.c | 2 +- > hw/block/Makefile.objs | 2 +- > 7 files changed, 29 insertions(+), 14 deletions(-) Thanks, applied to my block tree with Paolo's Acked-by: https://github.com/stefanha/qemu/commits/block Stefan
© 2016 - 2024 Red Hat, Inc.