On 11/10/22 17:48, Emanuele Giuseppe Esposito wrote:
+ /* Remove all memslots before adding the new ones. */
+ QSIMPLEQ_FOREACH_SAFE(u1, &kml->transaction_del, next, u2) {
+ kvm_set_phys_mem(kml, u1->section, false);
+ memory_region_unref(u1->section->mr);
+
+ QSIMPLEQ_REMOVE(&kml->transaction_del, u1, KVMMemoryUpdate, next);
+ memory_region_section_free_copy(u1->section);
+ g_free(u1);
+ }
+ QSIMPLEQ_FOREACH_SAFE(u1, &kml->transaction_add, next, u2) {
+ memory_region_ref(u1->section->mr);
+ kvm_set_phys_mem(kml, u1->section, true);
+
+ QSIMPLEQ_REMOVE(&kml->transaction_add, u1, KVMMemoryUpdate, next);
+ memory_region_section_free_copy(u1->section);
+ g_free(u1);
+ }
I'm not a huge fan of new_copy/free_copy, and I don't think it's needed
here. The FlatView is certainly alive between begin and commit (see
address_space_set_flatview()), and the MemoryRegion is kept alive by the
FlatView. In other words, unlike other uses of the functions, here the
lifetime is bound by kvm_region_commit, and that means you can just copy
by value. You can just copy it into KVMMemoryUpdate, i.e.
typedef struct KVMMemoryUpdate {
QSIMPLEQ_ENTRY(KVMMemoryUpdate) next;
MemoryRegionSection section;
} KVMMemoryUpdate;
Also, you can write the loop as
while (!QSIMPLEQ_EMPTY(&kvm->transaction_add) {
u = QSIMPLEQ_FIRST(&kvm->transaction_add);
QSIMPLEQ_REMOVE_HEAD(&kml->transaction_add, next);
...
g_free(u);
}
This clarifies the invariant that the QSIMPLEQs become empty at the end
of the loop. If it were QTAILQ it would be more a matter of personal
taste, but QSIMPLEQ_REMOVE is technically not constant-time and that
also tilts in favor of QSIMPLEQ_REMOVE_HEAD.
In fact I think we should remove QSIMPLEQ_REMOVE and QSLIST_REMOVE,
changing all users that need it to QTAILQ and QLIST respectively...
added to https://wiki.qemu.org/Contribute/BiteSizedTasks#API_conversion.
Paolo
