[PATCH] memory: transaction API

[PATCH] memory: transaction API

[Avi Kivity]

Allow changes to the memory hierarchy to be accumulated and
made visible all at once.  This reduces computational effort,
especially when an accelerator (e.g. kvm) is involved.

Useful when a single register update causes multiple changes
to an address space.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 memory.c |   20 ++++++++++++++++++++
 memory.h |    8 ++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index 0ffc905..8780533 100644
--- a/memory.c
+++ b/memory.c
@@ -18,6 +18,8 @@
 #include "kvm.h"
 #include <assert.h>
 
+unsigned memory_region_transaction_depth = 0;
+
 typedef struct AddrRange AddrRange;
 
 struct AddrRange {
@@ -648,6 +650,10 @@ static void address_space_update_topology(AddressSpace *as)
 
 static void memory_region_update_topology(void)
 {
+    if (memory_region_transaction_depth) {
+        return;
+    }
+
     if (address_space_memory.root) {
         address_space_update_topology(&address_space_memory);
     }
@@ -656,6 +662,20 @@ static void memory_region_update_topology(void)
     }
 }
 
+void memory_region_transaction_begin(void)
+{
+    ++memory_region_transaction_depth;
+}
+
+void memory_region_transaction_commit(void)
+{
+    if (!memory_region_transaction_depth) {
+        abort();
+    }
+    --memory_region_transaction_depth;
+    memory_region_update_topology();
+}
+
 void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size)
diff --git a/memory.h b/memory.h
index e4c0ad1..cb3a9b6 100644
--- a/memory.h
+++ b/memory.h
@@ -246,6 +246,14 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
 void memory_region_del_subregion(MemoryRegion *mr,
                                  MemoryRegion *subregion);
 
+/* Start a transaction; changes will be accumulated and made visible only
+ * when the transaction ends.
+ */
+void memory_region_transaction_begin(void);
+/* Commit a transaction and make changes visible to the guest.
+ */
+void memory_region_transaction_commit(void);
+
 #endif
 
 #endif
-- 
1.7.5.3

Re: [PATCH] memory: transaction API

[Jan Kiszka]

On 2011-07-21 12:21, Avi Kivity wrote:
> Allow changes to the memory hierarchy to be accumulated and
> made visible all at once.  This reduces computational effort,
> especially when an accelerator (e.g. kvm) is involved.
> 
> Useful when a single register update causes multiple changes
> to an address space.

That's simple to implement in the core, but complicates life for the
users, at least for the simple "update this region" use case.

Do we have transactional scenarios during runtime where multiple memory
regions are reconfigured?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] memory: transaction API

[Ferry Huberts]

On 07/21/2011 12:21 PM, Avi Kivity wrote:
> Allow changes to the memory hierarchy to be accumulated and
> made visible all at once.  This reduces computational effort,
> especially when an accelerator (e.g. kvm) is involved.
> 
> Useful when a single register update causes multiple changes
> to an address space.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  memory.c |   20 ++++++++++++++++++++
>  memory.h |    8 ++++++++
>  2 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 0ffc905..8780533 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -18,6 +18,8 @@
>  #include "kvm.h"
>  #include <assert.h>
>  
> +unsigned memory_region_transaction_depth = 0;
> +
>  typedef struct AddrRange AddrRange;
>  
>  struct AddrRange {
> @@ -648,6 +650,10 @@ static void address_space_update_topology(AddressSpace *as)
>  
>  static void memory_region_update_topology(void)
>  {
> +    if (memory_region_transaction_depth) {
> +        return;
> +    }
> +
>      if (address_space_memory.root) {
>          address_space_update_topology(&address_space_memory);
>      }
> @@ -656,6 +662,20 @@ static void memory_region_update_topology(void)
>      }
>  }
>  
> +void memory_region_transaction_begin(void)
> +{
> +    ++memory_region_transaction_depth;
> +}
> +

wouldn't you rather keep it safe by doing either here

if (!memory_region_transaction_depth)
  memory_region_transaction_depth++;

> +void memory_region_transaction_commit(void)
> +{
> +    if (!memory_region_transaction_depth) {
> +        abort();
> +    }


> +    --memory_region_transaction_depth;
> +    memory_region_update_topology();
> +}
> +

or by doing here

while (!memory_region_transaction_depth)
  memory_region_transaction_depth--;


with your setup nesting transactions will not work correctly I think.
You seem to have designed it to not do nesting, so it's safer to make
that explicit in your code imho

therefore I'd go for the change in _begin

also, wouldn't you rather rename memory_region_transaction_depth to
memory_region_transaction_pending?

>  void memory_region_init(MemoryRegion *mr,
>                          const char *name,
>                          uint64_t size)
> diff --git a/memory.h b/memory.h
> index e4c0ad1..cb3a9b6 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -246,6 +246,14 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
>  void memory_region_del_subregion(MemoryRegion *mr,
>                                   MemoryRegion *subregion);
>  
> +/* Start a transaction; changes will be accumulated and made visible only
> + * when the transaction ends.
> + */
> +void memory_region_transaction_begin(void);
> +/* Commit a transaction and make changes visible to the guest.
> + */
> +void memory_region_transaction_commit(void);
> +
>  #endif
>  
>  #endif


-- 
Ferry Huberts

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 01:38 PM, Jan Kiszka wrote:
> On 2011-07-21 12:21, Avi Kivity wrote:
> >  Allow changes to the memory hierarchy to be accumulated and
> >  made visible all at once.  This reduces computational effort,
> >  especially when an accelerator (e.g. kvm) is involved.
> >
> >  Useful when a single register update causes multiple changes
> >  to an address space.
>
> That's simple to implement in the core, but complicates life for the
> users, at least for the simple "update this region" use case.
>

Why? just stick a _begin() and _commit() at the start and end of the 
update_mapping() function.  It's an optional API, for simple cases (like 
mapping a BAR) you don't have to use it.


> Do we have transactional scenarios during runtime where multiple memory
> regions are reconfigured?

Both cirrus and 440fx PAM, I believe.  They don't check for the "no 
change" condition (at least, not completely) and instead override the 
previous mapping.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 02:04 PM, Ferry Huberts wrote:
> On 07/21/2011 12:21 PM, Avi Kivity wrote:
> >  Allow changes to the memory hierarchy to be accumulated and
> >  made visible all at once.  This reduces computational effort,
> >  especially when an accelerator (e.g. kvm) is involved.
> >
> >  Useful when a single register update causes multiple changes
> >  to an address space.
> >
> >
> >  +void memory_region_transaction_begin(void)
> >  +{
> >  +    ++memory_region_transaction_depth;
> >  +}
> >  +
>
> wouldn't you rather keep it safe by doing either here
>
> if (!memory_region_transaction_depth)
>    memory_region_transaction_depth++;
>

Why? I want to allow nesting transactions (not that I anticipate such a 
case).

> >  +void memory_region_transaction_commit(void)
> >  +{
> >  +    if (!memory_region_transaction_depth) {
> >  +        abort();
> >  +    }
>
>
> >  +    --memory_region_transaction_depth;
> >  +    memory_region_update_topology();
> >  +}
> >  +
>
> or by doing here
>
> while (!memory_region_transaction_depth)
>    memory_region_transaction_depth--;
>
>
> with your setup nesting transactions will not work correctly I think.
> You seem to have designed it to not do nesting, so it's safer to make
> that explicit in your code imho

Nesting should work just fine.

> therefore I'd go for the change in _begin
>
> also, wouldn't you rather rename memory_region_transaction_depth to
> memory_region_transaction_pending?

The existing name works for me, but if people want it changed, that's 
fine too.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 03:05 PM, Avi Kivity wrote:
> On 07/21/2011 01:38 PM, Jan Kiszka wrote:
>> On 2011-07-21 12:21, Avi Kivity wrote:
>> >  Allow changes to the memory hierarchy to be accumulated and
>> >  made visible all at once.  This reduces computational effort,
>> >  especially when an accelerator (e.g. kvm) is involved.
>> >
>> >  Useful when a single register update causes multiple changes
>> >  to an address space.
>>
>> That's simple to implement in the core, but complicates life for the
>> users, at least for the simple "update this region" use case.
>>
>
> Why? just stick a _begin() and _commit() at the start and end of the 
> update_mapping() function.  It's an optional API, for simple cases 
> (like mapping a BAR) you don't have to use it.
>

Do you see an improvement if you change cirrus_update_memory_access() in 
this fashion?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] memory: transaction API

[Jan Kiszka]

On 2011-07-21 14:05, Avi Kivity wrote:
> On 07/21/2011 01:38 PM, Jan Kiszka wrote:
>> On 2011-07-21 12:21, Avi Kivity wrote:
>>>  Allow changes to the memory hierarchy to be accumulated and
>>>  made visible all at once.  This reduces computational effort,
>>>  especially when an accelerator (e.g. kvm) is involved.
>>>
>>>  Useful when a single register update causes multiple changes
>>>  to an address space.
>>
>> That's simple to implement in the core, but complicates life for the
>> users, at least for the simple "update this region" use case.
>>
> 
> Why? just stick a _begin() and _commit() at the start and end of the 
> update_mapping() function.  It's an optional API, for simple cases (like 
> mapping a BAR) you don't have to use it.

begin
delete old
free old
create new
add new
end

vs.

update

> 
> 
>> Do we have transactional scenarios during runtime where multiple memory
>> regions are reconfigured?
> 
> Both cirrus and 440fx PAM, I believe.  They don't check for the "no 
> change" condition (at least, not completely) and instead override the 
> previous mapping.

That's the job of the memory mapping core IIUC. But it can only be done
efficiently with an 'update' operation.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 03:09 PM, Jan Kiszka wrote:
> >
> >  Why? just stick a _begin() and _commit() at the start and end of the
> >  update_mapping() function.  It's an optional API, for simple cases (like
> >  mapping a BAR) you don't have to use it.
>
> begin
> delete old
> free old
> create new
> add new
> end
>
> vs.
>
> update
>

The problem is that "update" can change lots of things.  offset, size, 
whether it's mmio or RAM, read-onlyness, even the wierd things like 
coalesced mmio.  So it's either a function with 324.2 parameters (or a 
large struct), or it's a series of functions with demarcation as to 
where the update begins and ends.

> >
> >
> >>  Do we have transactional scenarios during runtime where multiple memory
> >>  regions are reconfigured?
> >
> >  Both cirrus and 440fx PAM, I believe.  They don't check for the "no
> >  change" condition (at least, not completely) and instead override the
> >  previous mapping.
>
> That's the job of the memory mapping core IIUC.

In my opinion, too.  Devices should be dead simple.

> But it can only be done
> efficiently with an 'update' operation.

Why is the transaction API inefficient? AFAICT it accomplishes the same 
thing.  Some cycles are spent on finding out nothing has changed, but 
that's fine.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] memory: transaction API

[Ferry Huberts]

On 07/21/2011 02:07 PM, Avi Kivity wrote:
> On 07/21/2011 02:04 PM, Ferry Huberts wrote:
>> On 07/21/2011 12:21 PM, Avi Kivity wrote:
>> >  Allow changes to the memory hierarchy to be accumulated and
>> >  made visible all at once.  This reduces computational effort,
>> >  especially when an accelerator (e.g. kvm) is involved.
>> >
>> >  Useful when a single register update causes multiple changes
>> >  to an address space.
>> >
>> >
>> >  +void memory_region_transaction_begin(void)
>> >  +{
>> >  +    ++memory_region_transaction_depth;
>> >  +}
>> >  +
>>
>> wouldn't you rather keep it safe by doing either here
>>
>> if (!memory_region_transaction_depth)
>>    memory_region_transaction_depth++;
>>
> 
> Why? I want to allow nesting transactions (not that I anticipate such a
> case).
> 
>> >  +void memory_region_transaction_commit(void)
>> >  +{
>> >  +    if (!memory_region_transaction_depth) {
>> >  +        abort();
>> >  +    }
>>
>>
>> >  +    --memory_region_transaction_depth;
>> >  +    memory_region_update_topology();
>> >  +}
>> >  +
>>
>> or by doing here
>>
>> while (!memory_region_transaction_depth)
>>    memory_region_transaction_depth--;

doesn't memory_region_update_topology commit all accumulated changes? if
it does then memory_region_transaction_depth is left non-zero in the
nesting case while no more changes are actually present, resulting in
superfluous calls to memory_region_update_topology.

maybe I misunderstood memory_region_update_topology?


>>
>>
>> with your setup nesting transactions will not work correctly I think.
>> You seem to have designed it to not do nesting, so it's safer to make
>> that explicit in your code imho
> 
> Nesting should work just fine.
> 
>> therefore I'd go for the change in _begin
>>
>> also, wouldn't you rather rename memory_region_transaction_depth to
>> memory_region_transaction_pending?
> 
> The existing name works for me, but if people want it changed, that's
> fine too.
> 
> 


-- 
Ferry Huberts

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 03:26 PM, Ferry Huberts wrote:
> >>  >   +void memory_region_transaction_begin(void)
> >>  >   +{
> >>  >   +    ++memory_region_transaction_depth;
> >>  >   +}
> >>  >   +
> >>
> >>  wouldn't you rather keep it safe by doing either here
> >>
> >>  if (!memory_region_transaction_depth)
> >>     memory_region_transaction_depth++;
> >>
> >
> >  Why? I want to allow nesting transactions (not that I anticipate such a
> >  case).
> >
>
> doesn't memory_region_update_topology commit all accumulated changes?

It does.

>   if
> it does then memory_region_transaction_depth is left non-zero in the
> nesting case while no more changes are actually present, resulting in
> superfluous calls to memory_region_update_topology.
>
> maybe I misunderstood memory_region_update_topology?
>

update_mapping()
{
m_r_t_begin();
// call memory API functions to change hierarchy
some_other_function() entered
   m_r_t_begin();
   // call more memory API functions to change hierarchy
   m_r_t_commit();  // nothing happens
some_other_function() exits
// call even more memory API functions to change hierarchy
m_r_t_commit();  // all accumulated changes become visible
}

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] memory: transaction API

[Jan Kiszka]

On 2011-07-21 14:13, Avi Kivity wrote:
> On 07/21/2011 03:09 PM, Jan Kiszka wrote:
>>>
>>>  Why? just stick a _begin() and _commit() at the start and end of the
>>>  update_mapping() function.  It's an optional API, for simple cases (like
>>>  mapping a BAR) you don't have to use it.
>>
>> begin
>> delete old
>> free old
>> create new
>> add new
>> end
>>
>> vs.
>>
>> update
>>
> 
> The problem is that "update" can change lots of things.  offset, size, 
> whether it's mmio or RAM, read-onlyness, even the wierd things like 
> coalesced mmio.  So it's either a function with 324.2 parameters (or a 
> large struct), or it's a series of functions with demarcation as to 
> where the update begins and ends.

We do not need to provide update support for each and every bit, but for
the common cases. memory_region_update_alias(region, offset, size) would
be an excellent first candidate IMO.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] memory: transaction API

[Ferry Huberts]

On 07/21/2011 02:46 PM, Avi Kivity wrote:
> On 07/21/2011 03:26 PM, Ferry Huberts wrote:
>> >>  >   +void memory_region_transaction_begin(void)
>> >>  >   +{
>> >>  >   +    ++memory_region_transaction_depth;
>> >>  >   +}
>> >>  >   +
>> >>
>> >>  wouldn't you rather keep it safe by doing either here
>> >>
>> >>  if (!memory_region_transaction_depth)
>> >>     memory_region_transaction_depth++;
>> >>
>> >
>> >  Why? I want to allow nesting transactions (not that I anticipate
>> such a
>> >  case).
>> >
>>
>> doesn't memory_region_update_topology commit all accumulated changes?
> 
> It does.
> 
>>   if
>> it does then memory_region_transaction_depth is left non-zero in the
>> nesting case while no more changes are actually present, resulting in
>> superfluous calls to memory_region_update_topology.
>>
>> maybe I misunderstood memory_region_update_topology?
>>
> 
> update_mapping()
> {
> m_r_t_begin();
> // call memory API functions to change hierarchy
> some_other_function() entered
>   m_r_t_begin();
>   // call more memory API functions to change hierarchy
>   m_r_t_commit();  // nothing happens
> some_other_function() exits
> // call even more memory API functions to change hierarchy
> m_r_t_commit();  // all accumulated changes become visible
> }
> 

ahhh. I completely read over the memory_region_update_topology change.
shame on me, sorry for the confusion!

-- 
Ferry Huberts

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 03:52 PM, Jan Kiszka wrote:
> >
> >  The problem is that "update" can change lots of things.  offset, size,
> >  whether it's mmio or RAM, read-onlyness, even the wierd things like
> >  coalesced mmio.  So it's either a function with 324.2 parameters (or a
> >  large struct), or it's a series of functions with demarcation as to
> >  where the update begins and ends.
>
> We do not need to provide update support for each and every bit, but for
> the common cases. memory_region_update_alias(region, offset, size) would
> be an excellent first candidate IMO.

It's not enough, look at cirrus and PAM.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] memory: transaction API

[Jan Kiszka]

On 2011-07-21 14:58, Avi Kivity wrote:
> On 07/21/2011 03:52 PM, Jan Kiszka wrote:
>>>
>>>  The problem is that "update" can change lots of things.  offset, size,
>>>  whether it's mmio or RAM, read-onlyness, even the wierd things like
>>>  coalesced mmio.  So it's either a function with 324.2 parameters (or a
>>>  large struct), or it's a series of functions with demarcation as to
>>>  where the update begins and ends.
>>
>> We do not need to provide update support for each and every bit, but for
>> the common cases. memory_region_update_alias(region, offset, size) would
>> be an excellent first candidate IMO.
> 
> It's not enough, look at cirrus and PAM.

It's a perfect fit for cirrus, but PAM indeed requires set_readonly in
addition.

I also think now that describing a memory region offline via a struct
and then passing that to an atomic add/del/update would be a more handy
and future-proof API than an increasing number set functions.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 04:17 PM, Jan Kiszka wrote:
> On 2011-07-21 14:58, Avi Kivity wrote:
> >  On 07/21/2011 03:52 PM, Jan Kiszka wrote:
> >>>
> >>>   The problem is that "update" can change lots of things.  offset, size,
> >>>   whether it's mmio or RAM, read-onlyness, even the wierd things like
> >>>   coalesced mmio.  So it's either a function with 324.2 parameters (or a
> >>>   large struct), or it's a series of functions with demarcation as to
> >>>   where the update begins and ends.
> >>
> >>  We do not need to provide update support for each and every bit, but for
> >>  the common cases. memory_region_update_alias(region, offset, size) would
> >>  be an excellent first candidate IMO.
> >
> >  It's not enough, look at cirrus and PAM.
>
> It's a perfect fit for cirrus, but PAM indeed requires set_readonly in
> addition.
>

It isn't a pefect fit for cirrus.  If the mode changes in a way that 
makes mapping the map as RAM possible, or vice versa, and if the banks 
are contiguous, then _update() results in two mappings or unmappings, 
while _commit() results in just one (since m_r_update_topology() merges 
the two adjacent regions).

> I also think now that describing a memory region offline via a struct
> and then passing that to an atomic add/del/update would be a more handy
> and future-proof API than an increasing number set functions.

Maybe.  But it's not sufficient for atomic changes involving multiple 
regions.

btw, there is another implementation issue involving SMP - if a region 
that obscures the middle of another region is removed, we'll have two 
regions removed and replaced with a larger one.  That causes some memory 
to be temporarily inaccessible.  I don't think it's a problem in 
practice, but if it is, we can fix it by stopping all vcpus if we detect 
this condition, and by adding an atomic 
change-memory-map-and-get-dirty-log ioctl to kvm.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] memory: transaction API

[Jan Kiszka]

On 2011-07-21 15:50, Avi Kivity wrote:
> On 07/21/2011 04:17 PM, Jan Kiszka wrote:
>> On 2011-07-21 14:58, Avi Kivity wrote:
>>>  On 07/21/2011 03:52 PM, Jan Kiszka wrote:
>>>>>
>>>>>   The problem is that "update" can change lots of things.  offset, size,
>>>>>   whether it's mmio or RAM, read-onlyness, even the wierd things like
>>>>>   coalesced mmio.  So it's either a function with 324.2 parameters (or a
>>>>>   large struct), or it's a series of functions with demarcation as to
>>>>>   where the update begins and ends.
>>>>
>>>>  We do not need to provide update support for each and every bit, but for
>>>>  the common cases. memory_region_update_alias(region, offset, size) would
>>>>  be an excellent first candidate IMO.
>>>
>>>  It's not enough, look at cirrus and PAM.
>>
>> It's a perfect fit for cirrus, but PAM indeed requires set_readonly in
>> addition.
>>
> 
> It isn't a pefect fit for cirrus.  If the mode changes in a way that 
> makes mapping the map as RAM possible, or vice versa, and if the banks 
> are contiguous, then _update() results in two mappings or unmappings, 
> while _commit() results in just one (since m_r_update_topology() merges 
> the two adjacent regions).

Continuous banks or mode changes are uncommon compared to offset changes
of the mapped window. Cirrus does not need to bother about continuity of
its banks (the memory core will), and mode changes could be implemented
by allowing updates of the priority, thus reordering the regions instead
of continuously deleting and recreating them.

> 
>> I also think now that describing a memory region offline via a struct
>> and then passing that to an atomic add/del/update would be a more handy
>> and future-proof API than an increasing number set functions.
> 
> Maybe.  But it's not sufficient for atomic changes involving multiple 
> regions.

Right. The question is still if there are use cases where this matters
(ie. update frequencies comparable to graphic scenarios).

> 
> btw, there is another implementation issue involving SMP - if a region 
> that obscures the middle of another region is removed, we'll have two 
> regions removed and replaced with a larger one.

Yes, I've seen many of such temporary changes in the cirrus remapping logs.

>  That causes some memory 
> to be temporarily inaccessible.  I don't think it's a problem in 
> practice, but if it is, we can fix it by stopping all vcpus if we detect 
> this condition, and by adding an atomic 
> change-memory-map-and-get-dirty-log ioctl to kvm.

I'm not sure if the cirrus or any similar hardware supports consistent
memory accesses during ongoing bank remappings (ie. while the CPU
issuing the remapping IO commands is blocked on QEMU executing them).
But such an IOCTL would resolve our problem with dropping a logged
region as well, right?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 05:32 PM, Jan Kiszka wrote:
> On 2011-07-21 15:50, Avi Kivity wrote:
> >  On 07/21/2011 04:17 PM, Jan Kiszka wrote:
> >>  On 2011-07-21 14:58, Avi Kivity wrote:
> >>>   On 07/21/2011 03:52 PM, Jan Kiszka wrote:
> >>>>>
> >>>>>    The problem is that "update" can change lots of things.  offset, size,
> >>>>>    whether it's mmio or RAM, read-onlyness, even the wierd things like
> >>>>>    coalesced mmio.  So it's either a function with 324.2 parameters (or a
> >>>>>    large struct), or it's a series of functions with demarcation as to
> >>>>>    where the update begins and ends.
> >>>>
> >>>>   We do not need to provide update support for each and every bit, but for
> >>>>   the common cases. memory_region_update_alias(region, offset, size) would
> >>>>   be an excellent first candidate IMO.
> >>>
> >>>   It's not enough, look at cirrus and PAM.
> >>
> >>  It's a perfect fit for cirrus, but PAM indeed requires set_readonly in
> >>  addition.
> >>
> >
> >  It isn't a pefect fit for cirrus.  If the mode changes in a way that
> >  makes mapping the map as RAM possible, or vice versa, and if the banks
> >  are contiguous, then _update() results in two mappings or unmappings,
> >  while _commit() results in just one (since m_r_update_topology() merges
> >  the two adjacent regions).
>
> Continuous banks or mode changes are uncommon compared to offset changes
> of the mapped window. Cirrus does not need to bother about continuity of
> its banks (the memory core will), and mode changes could be implemented
> by allowing updates of the priority, thus reordering the regions instead
> of continuously deleting and recreating them.

The point is _update() can only make changes for one region atomic, 
while _commit() is more general.  You can sometimes batch all changes 
into a single container region, but sometimes it is clumsy, and 
sometimes impossible.

Deletion and creation are needed because we can't update an alias' 
offset.  I guess I can add that functionality.  But it still isn't as 
general as _commit().

> >
> >>  I also think now that describing a memory region offline via a struct
> >>  and then passing that to an atomic add/del/update would be a more handy
> >>  and future-proof API than an increasing number set functions.
> >
> >  Maybe.  But it's not sufficient for atomic changes involving multiple
> >  regions.
>
> Right. The question is still if there are use cases where this matters
> (ie. update frequencies comparable to graphic scenarios).

Does even cirrus update this often?  I would guess cirrus usually uses 
the linear framebuffer, no?

I added support for aliases and the vga banks just to get Windows XP to 
clear the screen quickly on bootup (used to take ~10 seconds).rary 
changes in the cirrus remapping logs.

> >   That causes some memory
> >  to be temporarily inaccessible.  I don't think it's a problem in
> >  practice, but if it is, we can fix it by stopping all vcpus if we detect
> >  this condition, and by adding an atomic
> >  change-memory-map-and-get-dirty-log ioctl to kvm.
>
> I'm not sure if the cirrus or any similar hardware supports consistent
> memory accesses during ongoing bank remappings (ie. while the CPU
> issuing the remapping IO commands is blocked on QEMU executing them).

Point is, unaffected regions (and so unaffected devices) are also 
modified.  Consider a PAM modified from PCI to RAM.  The adjacent RAM 
regions are removed and re-added.  If some code on another cpu is 
running on this RAM, it would be a little confused.

The CPU that is issuing the command is unaffected.

> But such an IOCTL would resolve our problem with dropping a logged
> region as well, right?

Yes, if done right.  Still we need to support older kernels.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] memory: transaction API

[Jan Kiszka]

On 2011-07-21 16:39, Avi Kivity wrote:
> On 07/21/2011 05:32 PM, Jan Kiszka wrote:
>> On 2011-07-21 15:50, Avi Kivity wrote:
>>>  On 07/21/2011 04:17 PM, Jan Kiszka wrote:
>>>>  On 2011-07-21 14:58, Avi Kivity wrote:
>>>>>   On 07/21/2011 03:52 PM, Jan Kiszka wrote:
>>>>>>>
>>>>>>>    The problem is that "update" can change lots of things.  offset, size,
>>>>>>>    whether it's mmio or RAM, read-onlyness, even the wierd things like
>>>>>>>    coalesced mmio.  So it's either a function with 324.2 parameters (or a
>>>>>>>    large struct), or it's a series of functions with demarcation as to
>>>>>>>    where the update begins and ends.
>>>>>>
>>>>>>   We do not need to provide update support for each and every bit, but for
>>>>>>   the common cases. memory_region_update_alias(region, offset, size) would
>>>>>>   be an excellent first candidate IMO.
>>>>>
>>>>>   It's not enough, look at cirrus and PAM.
>>>>
>>>>  It's a perfect fit for cirrus, but PAM indeed requires set_readonly in
>>>>  addition.
>>>>
>>>
>>>  It isn't a pefect fit for cirrus.  If the mode changes in a way that
>>>  makes mapping the map as RAM possible, or vice versa, and if the banks
>>>  are contiguous, then _update() results in two mappings or unmappings,
>>>  while _commit() results in just one (since m_r_update_topology() merges
>>>  the two adjacent regions).
>>
>> Continuous banks or mode changes are uncommon compared to offset changes
>> of the mapped window. Cirrus does not need to bother about continuity of
>> its banks (the memory core will), and mode changes could be implemented
>> by allowing updates of the priority, thus reordering the regions instead
>> of continuously deleting and recreating them.
> 
> The point is _update() can only make changes for one region atomic, 
> while _commit() is more general.  You can sometimes batch all changes 
> into a single container region, but sometimes it is clumsy, and 
> sometimes impossible.
> 
> Deletion and creation are needed because we can't update an alias' 
> offset.  I guess I can add that functionality.  But it still isn't as 
> general as _commit().

OK. What about providing _update wrappers? They could be implemented
internally by the memory API in terms of begin - remove region - change
region object - re-add region - end. That would avoid boilerplate code
on the user side and still keep the option to do open-coded transaction
also outside the core.

> 
>>>
>>>>  I also think now that describing a memory region offline via a struct
>>>>  and then passing that to an atomic add/del/update would be a more handy
>>>>  and future-proof API than an increasing number set functions.
>>>
>>>  Maybe.  But it's not sufficient for atomic changes involving multiple
>>>  regions.
>>
>> Right. The question is still if there are use cases where this matters
>> (ie. update frequencies comparable to graphic scenarios).
> 
> Does even cirrus update this often?  I would guess cirrus usually uses 
> the linear framebuffer, no?

Not sure how this mode is called, but when vram is mapped linearly into
the 0xa0000 range via two 32K banks, you get quite a few updates on
larger screen changes.

> 
> I added support for aliases and the vga banks just to get Windows XP to 
> clear the screen quickly on bootup (used to take ~10 seconds).rary 
> changes in the cirrus remapping logs.
> 
>>>   That causes some memory
>>>  to be temporarily inaccessible.  I don't think it's a problem in
>>>  practice, but if it is, we can fix it by stopping all vcpus if we detect
>>>  this condition, and by adding an atomic
>>>  change-memory-map-and-get-dirty-log ioctl to kvm.
>>
>> I'm not sure if the cirrus or any similar hardware supports consistent
>> memory accesses during ongoing bank remappings (ie. while the CPU
>> issuing the remapping IO commands is blocked on QEMU executing them).
> 
> Point is, unaffected regions (and so unaffected devices) are also 
> modified.  Consider a PAM modified from PCI to RAM.  The adjacent RAM 
> regions are removed and re-added.  If some code on another cpu is 
> running on this RAM, it would be a little confused.
> 
> The CPU that is issuing the command is unaffected.
> 
>> But such an IOCTL would resolve our problem with dropping a logged
>> region as well, right?
> 
> Yes, if done right.  Still we need to support older kernels.

We will need workarounds like we have today, e.g. confining PAM memory
region fragmentation to certain patterns that known OSes require. Full,
correct support would remain the privilege of host kernels that allow to
combine multi-region updates to an atomic operation (+ returning or
preserving dirty logs).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] memory: transaction API

[Jan Kiszka]

On 2011-07-21 16:39, Avi Kivity wrote:
> On 07/21/2011 05:32 PM, Jan Kiszka wrote:
>> On 2011-07-21 15:50, Avi Kivity wrote:
>>>  On 07/21/2011 04:17 PM, Jan Kiszka wrote:
>>>>  On 2011-07-21 14:58, Avi Kivity wrote:
>>>>>   On 07/21/2011 03:52 PM, Jan Kiszka wrote:
>>>>>>>
>>>>>>>    The problem is that "update" can change lots of things.  offset, size,
>>>>>>>    whether it's mmio or RAM, read-onlyness, even the wierd things like
>>>>>>>    coalesced mmio.  So it's either a function with 324.2 parameters (or a
>>>>>>>    large struct), or it's a series of functions with demarcation as to
>>>>>>>    where the update begins and ends.
>>>>>>
>>>>>>   We do not need to provide update support for each and every bit, but for
>>>>>>   the common cases. memory_region_update_alias(region, offset, size) would
>>>>>>   be an excellent first candidate IMO.
>>>>>
>>>>>   It's not enough, look at cirrus and PAM.
>>>>
>>>>  It's a perfect fit for cirrus, but PAM indeed requires set_readonly in
>>>>  addition.
>>>>
>>>
>>>  It isn't a pefect fit for cirrus.  If the mode changes in a way that
>>>  makes mapping the map as RAM possible, or vice versa, and if the banks
>>>  are contiguous, then _update() results in two mappings or unmappings,
>>>  while _commit() results in just one (since m_r_update_topology() merges
>>>  the two adjacent regions).
>>
>> Continuous banks or mode changes are uncommon compared to offset changes
>> of the mapped window. Cirrus does not need to bother about continuity of
>> its banks (the memory core will), and mode changes could be implemented
>> by allowing updates of the priority, thus reordering the regions instead
>> of continuously deleting and recreating them.
> 
> The point is _update() can only make changes for one region atomic, 
> while _commit() is more general.  You can sometimes batch all changes 
> into a single container region, but sometimes it is clumsy, and 
> sometimes impossible.
> 
> Deletion and creation are needed because we can't update an alias' 
> offset.  I guess I can add that functionality.  But it still isn't as 
> general as _commit().

OK. What about providing _update wrappers? They could be implemented
internally by the memory API in terms of begin - remove region - change
region object - re-add region - end. That would avoid boilerplate code
on the user side and still keep the option to do open-coded transaction
also outside the core.

> 
>>>
>>>>  I also think now that describing a memory region offline via a struct
>>>>  and then passing that to an atomic add/del/update would be a more handy
>>>>  and future-proof API than an increasing number set functions.
>>>
>>>  Maybe.  But it's not sufficient for atomic changes involving multiple
>>>  regions.
>>
>> Right. The question is still if there are use cases where this matters
>> (ie. update frequencies comparable to graphic scenarios).
> 
> Does even cirrus update this often?  I would guess cirrus usually uses 
> the linear framebuffer, no?

Not sure how this mode is called, but when vram is mapped linearly into
the 0xa0000 range via two 32K banks, you get quite a few updates on
larger screen changes.

> 
> I added support for aliases and the vga banks just to get Windows XP to 
> clear the screen quickly on bootup (used to take ~10 seconds).rary 
> changes in the cirrus remapping logs.
> 
>>>   That causes some memory
>>>  to be temporarily inaccessible.  I don't think it's a problem in
>>>  practice, but if it is, we can fix it by stopping all vcpus if we detect
>>>  this condition, and by adding an atomic
>>>  change-memory-map-and-get-dirty-log ioctl to kvm.
>>
>> I'm not sure if the cirrus or any similar hardware supports consistent
>> memory accesses during ongoing bank remappings (ie. while the CPU
>> issuing the remapping IO commands is blocked on QEMU executing them).
> 
> Point is, unaffected regions (and so unaffected devices) are also 
> modified.  Consider a PAM modified from PCI to RAM.  The adjacent RAM 
> regions are removed and re-added.  If some code on another cpu is 
> running on this RAM, it would be a little confused.
> 
> The CPU that is issuing the command is unaffected.
> 
>> But such an IOCTL would resolve our problem with dropping a logged
>> region as well, right?
> 
> Yes, if done right.  Still we need to support older kernels.

We will need workarounds like we have today, e.g. confining PAM memory
region fragmentation to certain patterns that known OSes require. Full,
correct support would remain the privilege of host kernels that allow to
combine multi-region updates to an atomic operation (+ returning or
preserving dirty logs).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 06:05 PM, Jan Kiszka wrote:
> >
> >  The point is _update() can only make changes for one region atomic,
> >  while _commit() is more general.  You can sometimes batch all changes
> >  into a single container region, but sometimes it is clumsy, and
> >  sometimes impossible.
> >
> >  Deletion and creation are needed because we can't update an alias'
> >  offset.  I guess I can add that functionality.  But it still isn't as
> >  general as _commit().
>
> OK. What about providing _update wrappers? They could be implemented
> internally by the memory API in terms of begin - remove region - change
> region object - re-add region - end. That would avoid boilerplate code
> on the user side and still keep the option to do open-coded transaction
> also outside the core.

It's pretty easy to do updates without resorting to transactions.  If 
it's just some property, you simply call m_r_update_topology().  Things 
like priority need a bit more logic to keep the list in sorted order, 
but it's not really difficult.

> >
> >>>
> >>>>   I also think now that describing a memory region offline via a struct
> >>>>   and then passing that to an atomic add/del/update would be a more handy
> >>>>   and future-proof API than an increasing number set functions.
> >>>
> >>>   Maybe.  But it's not sufficient for atomic changes involving multiple
> >>>   regions.
> >>
> >>  Right. The question is still if there are use cases where this matters
> >>  (ie. update frequencies comparable to graphic scenarios).
> >
> >  Does even cirrus update this often?  I would guess cirrus usually uses
> >  the linear framebuffer, no?
>
> Not sure how this mode is called, but when vram is mapped linearly into
> the 0xa0000 range via two 32K banks, you get quite a few updates on
> larger screen changes.
>

Ok.  grub2 again?  or another guest?

> >
> >>  But such an IOCTL would resolve our problem with dropping a logged
> >>  region as well, right?
> >
> >  Yes, if done right.  Still we need to support older kernels.
>
> We will need workarounds like we have today, e.g. confining PAM memory
> region fragmentation to certain patterns that known OSes require. Full,
> correct support would remain the privilege of host kernels that allow to
> combine multi-region updates to an atomic operation (+ returning or
> preserving dirty logs).

An atomic update + dirty log fetch can be emulated by temporarily 
freezing all vcpus.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: transaction API

[Avi Kivity]

On 07/21/2011 06:05 PM, Jan Kiszka wrote:
> >
> >  The point is _update() can only make changes for one region atomic,
> >  while _commit() is more general.  You can sometimes batch all changes
> >  into a single container region, but sometimes it is clumsy, and
> >  sometimes impossible.
> >
> >  Deletion and creation are needed because we can't update an alias'
> >  offset.  I guess I can add that functionality.  But it still isn't as
> >  general as _commit().
>
> OK. What about providing _update wrappers? They could be implemented
> internally by the memory API in terms of begin - remove region - change
> region object - re-add region - end. That would avoid boilerplate code
> on the user side and still keep the option to do open-coded transaction
> also outside the core.

It's pretty easy to do updates without resorting to transactions.  If 
it's just some property, you simply call m_r_update_topology().  Things 
like priority need a bit more logic to keep the list in sorted order, 
but it's not really difficult.

> >
> >>>
> >>>>   I also think now that describing a memory region offline via a struct
> >>>>   and then passing that to an atomic add/del/update would be a more handy
> >>>>   and future-proof API than an increasing number set functions.
> >>>
> >>>   Maybe.  But it's not sufficient for atomic changes involving multiple
> >>>   regions.
> >>
> >>  Right. The question is still if there are use cases where this matters
> >>  (ie. update frequencies comparable to graphic scenarios).
> >
> >  Does even cirrus update this often?  I would guess cirrus usually uses
> >  the linear framebuffer, no?
>
> Not sure how this mode is called, but when vram is mapped linearly into
> the 0xa0000 range via two 32K banks, you get quite a few updates on
> larger screen changes.
>

Ok.  grub2 again?  or another guest?

> >
> >>  But such an IOCTL would resolve our problem with dropping a logged
> >>  region as well, right?
> >
> >  Yes, if done right.  Still we need to support older kernels.
>
> We will need workarounds like we have today, e.g. confining PAM memory
> region fragmentation to certain patterns that known OSes require. Full,
> correct support would remain the privilege of host kernels that allow to
> combine multi-region updates to an atomic operation (+ returning or
> preserving dirty logs).

An atomic update + dirty log fetch can be emulated by temporarily 
freezing all vcpus.


-- 
error compiling committee.c: too many arguments to function