[PATCH] KVM: Start lock documentation

[PATCH] KVM: Start lock documentation

[Jan Kiszka]

The goal of this document shall be
- overview of all locks used in KVM core
- provide details on the scope of each lock
- explain the lock type, specifically of a raw spin locks
- provide a lock ordering guide

Start with one dependency chain and two locks.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 Documentation/kvm/locking.txt |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/kvm/locking.txt

diff --git a/Documentation/kvm/locking.txt b/Documentation/kvm/locking.txt
new file mode 100644
index 0000000..23f9092
--- /dev/null
+++ b/Documentation/kvm/locking.txt
@@ -0,0 +1,30 @@
+KVM Lock Overview
+=================
+
+1. Acquisition Orders
+---------------------
+
+kvm_lock
++-> kvm::srcu / kvm::lock
+    +-> kvm::slots_lock
+        +-> kvm::mmu_lock
+...
+
+
+2. Reference
+------------
+
+Name:		kvm_lock
+Type:		raw_spinlock
+Arch:		any
+Protects:	- vm_list
+		- hardware virtualization enable/disable
+Comment:	'raw' because hardware enabling/disabling must be atomic /wrt
+		migration.
+
+Name:		kvm_arch::tsc_write_lock
+Type:		raw_spinlock
+Arch:		x86
+Protects:	- kvm_arch::{last_tsc_write,last_tsc_nsec,last_tsc_offset}
+		- tsc offset in vmcb
+Comment:	'raw' because updating the tsc offsets must not be preempted.
-- 
1.7.1

Re: [PATCH] KVM: Start lock documentation

[Jan Kiszka]

On 2011-02-09 15:11, Jan Kiszka wrote:
> The goal of this document shall be
> - overview of all locks used in KVM core
> - provide details on the scope of each lock
> - explain the lock type, specifically of a raw spin locks
> - provide a lock ordering guide
> 
> Start with one dependency chain and two locks.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  Documentation/kvm/locking.txt |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/kvm/locking.txt
> 
> diff --git a/Documentation/kvm/locking.txt b/Documentation/kvm/locking.txt
> new file mode 100644
> index 0000000..23f9092
> --- /dev/null
> +++ b/Documentation/kvm/locking.txt
> @@ -0,0 +1,30 @@
> +KVM Lock Overview
> +=================
> +
> +1. Acquisition Orders
> +---------------------
> +
> +kvm_lock
> ++-> kvm::srcu / kvm::lock
> +    +-> kvm::slots_lock
> +        +-> kvm::mmu_lock
> +...
> +
> +
> +2. Reference
> +------------
> +
> +Name:		kvm_lock
> +Type:		raw_spinlock
> +Arch:		any
> +Protects:	- vm_list
> +		- hardware virtualization enable/disable
> +Comment:	'raw' because hardware enabling/disabling must be atomic /wrt
> +		migration.
> +
> +Name:		kvm_arch::tsc_write_lock
> +Type:		raw_spinlock
> +Arch:		x86
> +Protects:	- kvm_arch::{last_tsc_write,last_tsc_nsec,last_tsc_offset}
> +		- tsc offset in vmcb
> +Comment:	'raw' because updating the tsc offsets must not be preempted.

Sorry, Zach, you should likely have a look at this as well.

Jan

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

Re: [PATCH] KVM: Start lock documentation

[Marcelo Tosatti]

On Wed, Feb 09, 2011 at 03:11:28PM +0100, Jan Kiszka wrote:
> The goal of this document shall be
> - overview of all locks used in KVM core
> - provide details on the scope of each lock
> - explain the lock type, specifically of a raw spin locks
> - provide a lock ordering guide
> 
> Start with one dependency chain and two locks.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  Documentation/kvm/locking.txt |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/kvm/locking.txt
> 
> diff --git a/Documentation/kvm/locking.txt b/Documentation/kvm/locking.txt
> new file mode 100644
> index 0000000..23f9092
> --- /dev/null
> +++ b/Documentation/kvm/locking.txt
> @@ -0,0 +1,30 @@
> +KVM Lock Overview
> +=================
> +
> +1. Acquisition Orders
> +---------------------
> +
> +kvm_lock
> ++-> kvm::srcu / kvm::lock
> +    +-> kvm::slots_lock
> +        +-> kvm::mmu_lock
> +...

Its not easy to understand what you mean here. What kvm_lock has to do
with the ordering described below it?

Other than that, "Reference" section structure is fine... (yeah, Avi has
been asking it for ages).

> +
> +2. Reference
> +------------
> +
> +Name:		kvm_lock
> +Type:		raw_spinlock
> +Arch:		any
> +Protects:	- vm_list
> +		- hardware virtualization enable/disable
> +Comment:	'raw' because hardware enabling/disabling must be atomic /wrt
> +		migration.
> +
> +Name:		kvm_arch::tsc_write_lock
> +Type:		raw_spinlock
> +Arch:		x86
> +Protects:	- kvm_arch::{last_tsc_write,last_tsc_nsec,last_tsc_offset}
> +		- tsc offset in vmcb
> +Comment:	'raw' because updating the tsc offsets must not be preempted.
> -- 
> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] KVM: Start lock documentation

[Avi Kivity]

On 02/15/2011 06:44 PM, Marcelo Tosatti wrote:
> On Wed, Feb 09, 2011 at 03:11:28PM +0100, Jan Kiszka wrote:
>> The goal of this document shall be
>> - overview of all locks used in KVM core
>> - provide details on the scope of each lock
>> - explain the lock type, specifically of a raw spin locks
>> - provide a lock ordering guide
>>
>> Start with one dependency chain and two locks.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> ---
>>   Documentation/kvm/locking.txt |   30 ++++++++++++++++++++++++++++++
>>   1 files changed, 30 insertions(+), 0 deletions(-)
>>   create mode 100644 Documentation/kvm/locking.txt
>>
>> diff --git a/Documentation/kvm/locking.txt b/Documentation/kvm/locking.txt
>> new file mode 100644
>> index 0000000..23f9092
>> --- /dev/null
>> +++ b/Documentation/kvm/locking.txt
>> @@ -0,0 +1,30 @@
>> +KVM Lock Overview
>> +=================
>> +
>> +1. Acquisition Orders
>> +---------------------
>> +
>> +kvm_lock
>> ++->  kvm::srcu / kvm::lock
>> +    +->  kvm::slots_lock
>> +        +->  kvm::mmu_lock
>> +...
> Its not easy to understand what you mean here. What kvm_lock has to do
> with the ordering described below it?
>

Yeah, I meant to make the same comment but forgot.  I suggest a union of 
paths:

   kvm::srcu (read) -> { fields }
| kvm::slots_lock(write) -> { fields }

but let's start with the reference section.

Re: [PATCH] KVM: Start lock documentation

[Jan Kiszka]

On 2011-02-15 17:44, Marcelo Tosatti wrote:
> On Wed, Feb 09, 2011 at 03:11:28PM +0100, Jan Kiszka wrote:
>> The goal of this document shall be
>> - overview of all locks used in KVM core
>> - provide details on the scope of each lock
>> - explain the lock type, specifically of a raw spin locks
>> - provide a lock ordering guide
>>
>> Start with one dependency chain and two locks.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  Documentation/kvm/locking.txt |   30 ++++++++++++++++++++++++++++++
>>  1 files changed, 30 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/kvm/locking.txt
>>
>> diff --git a/Documentation/kvm/locking.txt b/Documentation/kvm/locking.txt
>> new file mode 100644
>> index 0000000..23f9092
>> --- /dev/null
>> +++ b/Documentation/kvm/locking.txt
>> @@ -0,0 +1,30 @@
>> +KVM Lock Overview
>> +=================
>> +
>> +1. Acquisition Orders
>> +---------------------
>> +
>> +kvm_lock
>> ++-> kvm::srcu / kvm::lock
>> +    +-> kvm::slots_lock
>> +        +-> kvm::mmu_lock
>> +...
> 
> Its not easy to understand what you mean here. What kvm_lock has to do
> with the ordering described below it?

kvm_lock is the head of this chain, i.e. there are code paths where it
is taken first, then kvm::lock, etc.

Jan

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

Re: [PATCH] KVM: Start lock documentation

[Avi Kivity]

On 02/15/2011 07:08 PM, Jan Kiszka wrote:
> On 2011-02-15 17:44, Marcelo Tosatti wrote:
> >  On Wed, Feb 09, 2011 at 03:11:28PM +0100, Jan Kiszka wrote:
> >>  The goal of this document shall be
> >>  - overview of all locks used in KVM core
> >>  - provide details on the scope of each lock
> >>  - explain the lock type, specifically of a raw spin locks
> >>  - provide a lock ordering guide
> >>
> >>  Start with one dependency chain and two locks.
> >>
> >>  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>  ---
> >>   Documentation/kvm/locking.txt |   30 ++++++++++++++++++++++++++++++
> >>   1 files changed, 30 insertions(+), 0 deletions(-)
> >>   create mode 100644 Documentation/kvm/locking.txt
> >>
> >>  diff --git a/Documentation/kvm/locking.txt b/Documentation/kvm/locking.txt
> >>  new file mode 100644
> >>  index 0000000..23f9092
> >>  --- /dev/null
> >>  +++ b/Documentation/kvm/locking.txt
> >>  @@ -0,0 +1,30 @@
> >>  +KVM Lock Overview
> >>  +=================
> >>  +
> >>  +1. Acquisition Orders
> >>  +---------------------
> >>  +
> >>  +kvm_lock
> >>  ++->  kvm::srcu / kvm::lock
> >>  +    +->  kvm::slots_lock
> >>  +        +->  kvm::mmu_lock
> >>  +...
> >
> >  Its not easy to understand what you mean here. What kvm_lock has to do
> >  with the ordering described below it?
>
> kvm_lock is the head of this chain, i.e. there are code paths where it
> is taken first, then kvm::lock, etc.
>

Right, but it is not mandatory for most fields protected by the lock.

So we have
  - which locks _may_ nest, and how
  - which locks _must_ nest, and how, to access a particular field (may 
depend on type of access for rcu)

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

Re: [PATCH] KVM: Start lock documentation

[Avi Kivity]

On 02/09/2011 04:11 PM, Jan Kiszka wrote:
> The goal of this document shall be
> - overview of all locks used in KVM core
> - provide details on the scope of each lock
> - explain the lock type, specifically of a raw spin locks
> - provide a lock ordering guide
>
> Start with one dependency chain and two locks.
>

Applied, with the dependency chain omitted, thanks.

Future patches which affect locking will be required to update this 
document.

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