qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Juan Quintela <quintela@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Cornelia Huck" <cohuck@redhat.com>,
	"Alex Williamson" <alex.williamson@redhat.com>,
	qemu-block@nongnu.org, "David Hildenbrand" <david@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	qemu-devel@nongnu.org, "Max Reitz" <mreitz@redhat.com>,
	"Halil Pasic" <pasic@linux.ibm.com>,
	"Christian Borntraeger" <borntraeger@de.ibm.com>,
	qemu-s390x@nongnu.org, qemu-ppc@nongnu.org,
	"Keith Busch" <kbusch@kernel.org>,
	"Richard Henderson" <rth@twiddle.net>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	"David Gibson" <david@gibson.dropbear.id.au>
Subject: Re: [PATCH 00/10] exec: Shear 'exec/ram_addr.h' and make NVMe device target-agnostic
Date: Fri, 08 May 2020 10:21:32 +0200	[thread overview]
Message-ID: <871rnug7lf.fsf@secure.mitica> (raw)
In-Reply-To: <6109af19-24d0-1fdc-4f5c-772cfe9ec087@redhat.com> (Paolo Bonzini's message of "Fri, 8 May 2020 00:15:07 +0200")

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 07/05/20 19:39, Philippe Mathieu-Daudé wrote:
>> Stefan suggested to make qemu_ram_writeback() target agnostic,
>> Paolo to add memory_region_msync(), and Peter to remove
>> "exec/ram_addr.h" [*].
>> 
>> I let a single function in this file,
>> cpu_physical_memory_sync_dirty_bitmap(), to let the maintainer
>> have the pleasure to remove this header definitively himself :)
>
> I don't think this is a good idea. :)
>
> "exec/ram_addr.h" is a good place for functions that work on ram-addr_t
> and/or RAMBlock data.  There should very few of these, since these are
> mostly an internal concept that should only be used for live migration.
>  You could:
>
> - figure out which files actually need to include exec/ram_addr.h.
> There's already very few of them.

ram_addr.h looks really "not dangerous", I think that I preffer the
memory-internal.h or whatever name that implies that you should think
twice before using that file.

My main problem with that include are:

- cpu_physical_memory_set_dirty_lebitmap()
- cpu_physical_memory_sync_dirty_bitmap()

Both are long, both are complex, and if one changes them, it is very
probably that you end breaking some random architecture in TCG (being
there, done that).

As you said, that functions are used only in a couple of places.  I
haven't meassured the impact of moving it to a .c file, but I would
preffer it if performance don't suffer.

>
> - move the large functions to a new .c file, ramblock.c.  Figure out
> which can be static, move the declarations for the others to ramblock.h

Ok, it appears that we kind of agree.

> - kill ram_addr.h and include ramblock.h instead.

I created ramblock.h initially (/me checks) because if you just need to
walk a ramblock (that is clearly target independent code), you needed to
become target dependent, due to the definitions that are there inside.

I don't care one way or another, just that we don't create the old
dependency.

> Not coincidentially, qemu_ram_writeback() takes a RAMBlock*, and it ends
> up in ramblock.h.

Thanks, Juan.



  reply	other threads:[~2020-05-08  8:22 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-07 17:39 [PATCH 00/10] exec: Shear 'exec/ram_addr.h' and make NVMe device target-agnostic Philippe Mathieu-Daudé
2020-05-07 17:39 ` [PATCH 01/10] exec: Rename qemu_ram_writeback() as qemu_ram_msync() Philippe Mathieu-Daudé
2020-05-08  7:59   ` Juan Quintela
2020-05-07 17:39 ` [PATCH 02/10] exec/ramblock: Add missing 'qemu/rcu.h' include Philippe Mathieu-Daudé
2020-05-08  7:59   ` Juan Quintela
2020-05-07 17:39 ` [PATCH 03/10] exec: Move tb_invalidate_phys_range() to 'exec/exec-all.h' Philippe Mathieu-Daudé
2020-05-07 17:39 ` [PATCH 04/10] exec/memory-internal: Check CONFIG_SOFTMMU instead of CONFIG_USER_ONLY Philippe Mathieu-Daudé
2020-05-08  8:01   ` Juan Quintela
2020-05-07 17:39 ` [PATCH 05/10] exec: Move qemu_minrampagesize/qemu_maxrampagesize to 'qemu-common.h' Philippe Mathieu-Daudé
2020-05-08  8:03   ` Juan Quintela
2020-05-08  9:09   ` Cornelia Huck
2020-05-11  1:32   ` David Gibson
2020-05-07 17:39 ` [PATCH 06/10] exec: Move ramblock_recv_bitmap_offset() to migration/ram.c Philippe Mathieu-Daudé
2020-05-08  8:07   ` Juan Quintela
2020-05-08  9:27     ` Philippe Mathieu-Daudé
2020-05-07 17:39 ` [PATCH 07/10] exec: Move all RAMBlock functions to 'exec/ramblock.h' Philippe Mathieu-Daudé
2020-05-08  8:08   ` Juan Quintela
2020-05-08  9:32   ` Cornelia Huck
2020-05-11  1:33   ` David Gibson
2020-05-07 17:39 ` [PATCH 08/10] hw/block: Let the NVMe emulated device be target-agnostic Philippe Mathieu-Daudé
2020-05-07 17:42   ` Philippe Mathieu-Daudé
2020-05-08  8:09   ` Juan Quintela
2020-05-07 17:39 ` [PATCH 09/10] exec: Update coding style to make checkpatch.pl happy Philippe Mathieu-Daudé
2020-05-08  9:05   ` Juan Quintela
2020-05-07 17:39 ` [PATCH 10/10] exec: Move cpu_physical_memory_* functions to 'exec/memory-internal.h' Philippe Mathieu-Daudé
2020-05-08  8:12   ` Juan Quintela
2020-05-11  1:35   ` David Gibson
2020-05-07 22:15 ` [PATCH 00/10] exec: Shear 'exec/ram_addr.h' and make NVMe device target-agnostic Paolo Bonzini
2020-05-08  8:21   ` Juan Quintela [this message]
2020-05-08  6:19 ` no-reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=871rnug7lf.fsf@secure.mitica \
    --to=quintela@redhat.com \
    --cc=alex.williamson@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=david@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=kbusch@kernel.org \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=mst@redhat.com \
    --cc=pasic@linux.ibm.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=philmd@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=qemu-s390x@nongnu.org \
    --cc=rth@twiddle.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).