Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Fam Zheng]

On Sat, 11/28 13:51, Peter Xu wrote:
> On Fri, Nov 27, 2015 at 01:14:25PM +0800, Fam Zheng wrote:
> > On Fri, 11/27 10:48, Peter Xu wrote:
> 
> [snip]
> 
> > 
> > This patch doesn't handle the incoming migration case, i.e. when QEMU is
> > started with "-incoming", or "-incoming defer". Dump can go wrong when 
> > incoming
> > migration happens at the same time.
> 
> Sorry to missed these lines. Still not understand why this patch
> cannot handle this if with [1] (Please check below for [1])?

You're right, that does work. I missed that "defer" also sets
RUN_STATE_INMIGRATE.

Fam

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Peter Xu]

On Fri, Nov 27, 2015 at 11:14:17AM +0100, Paolo Bonzini wrote:
> 
> 
> On 27/11/2015 07:27, Peter Xu wrote:
> > If embed the mr pointer into GuestPhyBlock, then we might need to
> > ref/unref one MemoryRegion for multiple times (and I am not sure how
> > many, maybe it could be a very big number especially the
> > MemoryRegionSections are scattered?). Not sure whether it is more
> > efficient.
> > 
> > For what I see, the number of MemoryRegions should be not big (<5 in
> > my case). So even with O(N^2), it should merely like O(N). Not sure
> > about this too.
> > 
> > Would like to hear more review comments from Paolo and others.
> > 
> 
> Fam suggestion is a good one, ref'ing one MemoryRegion many times is not
> a problem.

Ok, then I will embed the MemoryRegion pointer into GuestPhysBlocks
in v3.

> 
> Also I noticed now that you do the dump_init in the main thread (using a
> listener), so the RCU lock/unlock is not needed.  I don't know this code
> very well.

Yes, it's in main thread too in v1 patch, since I think that should
not be the time consuming part. If so, I will remove the RCU
lock/unlock too.

> 
> It's worth adding a comment at the top of functions that are called from
> a separate thread.

Ok. Will add that.

Thanks!
Peter

> 
> Thanks,
> 
> Paolo

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Peter Xu]

On Fri, Nov 27, 2015 at 11:31:00AM +0100, Paolo Bonzini wrote:
> 

[snip]

> > +
> > +static GlobalDumpState *dump_state_get_global(void)
> > +{
> > +static bool init = false;
> > +static GlobalDumpState global_dump_state;
> > +if (unlikely(!init)) {
> > +qemu_mutex_init(_dump_state.gds_mutex);
> 
> The mutex is not necessary.  The structure is always created in the main
> thread and released by the dump thread (of which there is just one).

[1]

> 
> You can then just make a global DumpState (not a pointer!), and separate
> the fields between:
> 
> - those that are protected by a mutex (most likely the DumpResult and
> written_size, possibly others)

Hi, Paolo,

So mutex is still necessary, right? (refer to [1]) Since
"dump-query" will read several fields of it, while the dump thread
might be modifying them as well?

> 
> - those that are only written before the thread is started, and thus do
> not need to be protected by a mutex
> 
> - those that are only accessed by the thread, and thus do not need to be
> protected by a mutex either.
> 
> See for example this extract from migration/block.c:
> 
> typedef struct BlkMigState {
> /* Written during setup phase.  Can be read without a lock.  */
> int blk_enable;
> int shared_base;
> QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
> int64_t total_sector_sum;
> bool zero_blocks;
> 
> /* Protected by lock.  */
> QSIMPLEQ_HEAD(blk_list, BlkMigBlock) blk_list;
> int submitted;
> int read_done;
> 
> /* Only used by migration thread.  Does not need a lock.  */
> int transferred;
> int prev_progress;
> int bulk_completed;
> 
> /* The lock.  */
> QemuMutex lock;
> } BlkMigState;
> 
> static BlkMigState block_mig_state;

Ok, I think I can remove the global state and make a static
DumpState. When I was drafting the patch, I just tried to keep the
old logic (malloc/free) and avoid introducing bugs. Maybe I was
wrong. I should better not introduce new struct if not necessary. 

Will try to follow this example in v3.

Thanks.
Peter

> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Paolo Bonzini]

On 27/11/2015 12:26, Peter Xu wrote:
> On Fri, Nov 27, 2015 at 11:31:00AM +0100, Paolo Bonzini wrote:
>>
> 
> [snip]
> 
>>> +
>>> +static GlobalDumpState *dump_state_get_global(void)
>>> +{
>>> +static bool init = false;
>>> +static GlobalDumpState global_dump_state;
>>> +if (unlikely(!init)) {
>>> +qemu_mutex_init(_dump_state.gds_mutex);
>>
>> The mutex is not necessary.  The structure is always created in the main
>> thread and released by the dump thread (of which there is just one).
> 
> [1]
> 
>>
>> You can then just make a global DumpState (not a pointer!), and separate
>> the fields between:
>>
>> - those that are protected by a mutex (most likely the DumpResult and
>> written_size, possibly others)
> 
> Hi, Paolo,
> 
> So mutex is still necessary, right? (refer to [1]) Since
> "dump-query" will read several fields of it, while the dump thread
> might be modifying them as well?

Right, initially I meant a mutex around the allocation.  But then I read
the other patches where you add more fields, and came back to add more
content.

Strictly speaking it's likely that you can do everything without a
mutex, but it's easier to use one.

> Ok, I think I can remove the global state and make a static
> DumpState. When I was drafting the patch, I just tried to keep the
> old logic (malloc/free) and avoid introducing bugs. Maybe I was
> wrong. I should better not introduce new struct if not necessary. 

It's okay.  The only confusing bit was that you had to add more state to
GlobalDumpState, and in the end it was split between DumpState and
GlobalDumpState.  As far as this patch is concerned, using malloc/free
would have been okay, but then the subsequent changes suggest a
different approach.

Thanks!

Paolo

> Will try to follow this example in v3.
> 
> Thanks.
> Peter
> 
>>
>> Paolo
>>
> 
> 

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Peter Xu]

On Fri, Nov 27, 2015 at 01:14:25PM +0800, Fam Zheng wrote:
> On Fri, 11/27 10:48, Peter Xu wrote:

[snip]

> 
> This patch doesn't handle the incoming migration case, i.e. when QEMU is
> started with "-incoming", or "-incoming defer". Dump can go wrong when 
> incoming
> migration happens at the same time.

Sorry to missed these lines. Still not understand why this patch
cannot handle this if with [1] (Please check below for [1])?

[snip]

> >  
> > +extern GlobalDumpState global_dump_state;
> 
> dump_state_get_global() returns a pointer to the local static variable, why do
> you need this?

Yes, I should remove that. Thanks!

[snip]

> > +
> > @@ -1590,6 +1674,14 @@ void qmp_dump_guest_memory(bool paging, const char 
> > *file,
> >  int fd = -1;
> >  DumpState *s;
> >  Error *local_err = NULL;
> > +/* by default, we are keeping the old style, which is sync dump */
> > +bool detach_p = false;
> > +GlobalDumpState *global = dump_state_get_global();
> > +

[1]

> > +if (runstate_check(RUN_STATE_INMIGRATE)) {
> > +error_setg(errp, "Dump not allowed during incoming migration.");
> > +return;
> > +}

Thanks.
Peter

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Paolo Bonzini]

On 27/11/2015 07:27, Peter Xu wrote:
> If embed the mr pointer into GuestPhyBlock, then we might need to
> ref/unref one MemoryRegion for multiple times (and I am not sure how
> many, maybe it could be a very big number especially the
> MemoryRegionSections are scattered?). Not sure whether it is more
> efficient.
> 
> For what I see, the number of MemoryRegions should be not big (<5 in
> my case). So even with O(N^2), it should merely like O(N). Not sure
> about this too.
> 
> Would like to hear more review comments from Paolo and others.
> 

Fam suggestion is a good one, ref'ing one MemoryRegion many times is not
a problem.

Also I noticed now that you do the dump_init in the main thread (using a
listener), so the RCU lock/unlock is not needed.  I don't know this code
very well.

It's worth adding a comment at the top of functions that are called from
a separate thread.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Paolo Bonzini]

On 27/11/2015 03:48, Peter Xu wrote:
> This patch implements "detach" for "dump-guest-memory" command. When
> specified, one background thread is created to do the dump work.
> 
> When there is a dump running in background, the commands "stop",
> "cont" and "dump-guest-memory" will fail directly, with a hint
> telling user: "there is a dump in progress".
> 
> Although it is not quite essential for now, the new codes are trying
> to make sure the dump is thread safe. To do this, one list is added
> into GuestPhysBlockList to track all the referenced MemoryRegions
> during dump process. We should make sure each MemoryRegion would
> only be referenced for once.
> 
> One global struct "GlobalDumpState" is added to store some global
> informations for dump procedures. One mutex is used to protect the
> global dump info.
> 
> Signed-off-by: Peter Xu 
> ---
>  dump.c  | 114 
> +---
>  include/qemu-common.h   |   4 ++
>  include/sysemu/dump.h   |  10 
>  include/sysemu/memory_mapping.h |   8 +++
>  memory_mapping.c|  46 +++-
>  qmp.c   |  14 +
>  6 files changed, 188 insertions(+), 8 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index d79e0ed..dfd56aa 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -73,6 +73,7 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>  static int dump_cleanup(DumpState *s)
>  {
>  guest_phys_blocks_free(>guest_phys_blocks);
> +guest_memory_region_free(>guest_phys_blocks);
>  memory_mapping_list_free(>list);
>  close(s->fd);
>  if (s->resume) {
> @@ -1427,6 +1428,9 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>  Error *err = NULL;
>  int ret;
>  
> +s->has_format = has_format;
> +s->format = format;
> +
>  /* kdump-compressed is conflict with paging and filter */
>  if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>  assert(!paging && !has_filter);
> @@ -1580,6 +1584,86 @@ cleanup:
>  dump_cleanup(s);
>  }
>  
> +extern GlobalDumpState global_dump_state;
> +
> +static GlobalDumpState *dump_state_get_global(void)
> +{
> +static bool init = false;
> +static GlobalDumpState global_dump_state;
> +if (unlikely(!init)) {
> +qemu_mutex_init(_dump_state.gds_mutex);

The mutex is not necessary.  The structure is always created in the main
thread and released by the dump thread (of which there is just one).

You can then just make a global DumpState (not a pointer!), and separate
the fields between:

- those that are protected by a mutex (most likely the DumpResult and
written_size, possibly others)

- those that are only written before the thread is started, and thus do
not need to be protected by a mutex

- those that are only accessed by the thread, and thus do not need to be
protected by a mutex either.

See for example this extract from migration/block.c:

typedef struct BlkMigState {
/* Written during setup phase.  Can be read without a lock.  */
int blk_enable;
int shared_base;
QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
int64_t total_sector_sum;
bool zero_blocks;

/* Protected by lock.  */
QSIMPLEQ_HEAD(blk_list, BlkMigBlock) blk_list;
int submitted;
int read_done;

/* Only used by migration thread.  Does not need a lock.  */
int transferred;
int prev_progress;
int bulk_completed;

/* The lock.  */
QemuMutex lock;
} BlkMigState;

static BlkMigState block_mig_state;

Paolo

> +global_dump_state.gds_cur = NULL;
> +init = true;
> +}
> +return _dump_state;
> +}
> +
> +/* Returns non-zero if there is existing dump in progress, otherwise
> + * zero is returned. */
> +bool dump_in_progress(void)
> +{
> +GlobalDumpState *global = dump_state_get_global();
> +/* we do not need to take the mutex here if we are checking the
> + * pointer only. */
> +return (!!global->gds_cur);
> +}
> +
> +/* Trying to create one DumpState. This will fail if there is an
> + * existing one. Return DumpState pointer if succeeded, otherwise
> + * NULL is returned. */
> +static DumpState *dump_state_create(GlobalDumpState *global)
> +{
> +DumpState *cur = NULL;
> +qemu_mutex_lock(>gds_mutex);
> +
> +cur = global->gds_cur;
> +if (cur) {
> +/* one dump in progress */
> +cur = NULL;
> +} else {
> +/* we are the first! */
> +cur = g_malloc0(sizeof(*cur));
> +global->gds_cur = cur;
> +}
> +
> +qemu_mutex_unlock(>gds_mutex);
> +return cur;
> +}
> +
> +/* release current DumpState structure */
> +static void dump_state_release(GlobalDumpState *global)
> +{
> +DumpState *cur = NULL;
> +qemu_mutex_lock(>gds_mutex);
> +
> +cur = global->gds_cur;
> +/* we should never call release before having one */
> +assert(cur);
> +global->gds_cur = NULL;
> +
> +

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Fam Zheng]

On Fri, 11/27 10:48, Peter Xu wrote:
> This patch implements "detach" for "dump-guest-memory" command. When
> specified, one background thread is created to do the dump work.
> 
> When there is a dump running in background, the commands "stop",
> "cont" and "dump-guest-memory" will fail directly, with a hint
> telling user: "there is a dump in progress".
> 
> Although it is not quite essential for now, the new codes are trying
> to make sure the dump is thread safe. To do this, one list is added
> into GuestPhysBlockList to track all the referenced MemoryRegions
> during dump process. We should make sure each MemoryRegion would
> only be referenced for once.
> 
> One global struct "GlobalDumpState" is added to store some global
> informations for dump procedures. One mutex is used to protect the
> global dump info.

This patch doesn't handle the incoming migration case, i.e. when QEMU is
started with "-incoming", or "-incoming defer". Dump can go wrong when incoming
migration happens at the same time.

> 
> Signed-off-by: Peter Xu 
> ---
>  dump.c  | 114 
> +---
>  include/qemu-common.h   |   4 ++
>  include/sysemu/dump.h   |  10 
>  include/sysemu/memory_mapping.h |   8 +++
>  memory_mapping.c|  46 +++-
>  qmp.c   |  14 +
>  6 files changed, 188 insertions(+), 8 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index d79e0ed..dfd56aa 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -73,6 +73,7 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>  static int dump_cleanup(DumpState *s)
>  {
>  guest_phys_blocks_free(>guest_phys_blocks);
> +guest_memory_region_free(>guest_phys_blocks);
>  memory_mapping_list_free(>list);
>  close(s->fd);
>  if (s->resume) {
> @@ -1427,6 +1428,9 @@ static void dump_init(DumpState *s, int fd, bool 
> has_format,
>  Error *err = NULL;
>  int ret;
>  
> +s->has_format = has_format;
> +s->format = format;
> +
>  /* kdump-compressed is conflict with paging and filter */
>  if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>  assert(!paging && !has_filter);
> @@ -1580,6 +1584,86 @@ cleanup:
>  dump_cleanup(s);
>  }
>  
> +extern GlobalDumpState global_dump_state;

dump_state_get_global() returns a pointer to the local static variable, why do
you need this?

But what I really wonder is why a single boolean is not enough: the DumpState
pointer can be passed to dump_thread, so it doesn't need to be global, then you
don't need the mutex.

> +
> +static GlobalDumpState *dump_state_get_global(void)
> +{
> +static bool init = false;
> +static GlobalDumpState global_dump_state;
> +if (unlikely(!init)) {
> +qemu_mutex_init(_dump_state.gds_mutex);
> +global_dump_state.gds_cur = NULL;
> +init = true;
> +}
> +return _dump_state;
> +}
> +
> +/* Returns non-zero if there is existing dump in progress, otherwise
> + * zero is returned. */
> +bool dump_in_progress(void)
> +{
> +GlobalDumpState *global = dump_state_get_global();
> +/* we do not need to take the mutex here if we are checking the
> + * pointer only. */
> +return (!!global->gds_cur);
> +}
> +
> +/* Trying to create one DumpState. This will fail if there is an
> + * existing one. Return DumpState pointer if succeeded, otherwise
> + * NULL is returned. */
> +static DumpState *dump_state_create(GlobalDumpState *global)
> +{
> +DumpState *cur = NULL;
> +qemu_mutex_lock(>gds_mutex);
> +
> +cur = global->gds_cur;
> +if (cur) {
> +/* one dump in progress */
> +cur = NULL;
> +} else {
> +/* we are the first! */
> +cur = g_malloc0(sizeof(*cur));
> +global->gds_cur = cur;
> +}
> +
> +qemu_mutex_unlock(>gds_mutex);
> +return cur;
> +}
> +
> +/* release current DumpState structure */
> +static void dump_state_release(GlobalDumpState *global)
> +{
> +DumpState *cur = NULL;
> +qemu_mutex_lock(>gds_mutex);
> +
> +cur = global->gds_cur;
> +/* we should never call release before having one */
> +assert(cur);
> +global->gds_cur = NULL;
> +
> +qemu_mutex_unlock(>gds_mutex);
> +dump_cleanup(cur);
> +g_free(cur);
> +}
> +
> +/* this operation might be time consuming. */
> +static void dump_process(DumpState *s, Error **errp)
> +{
> +if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
> +create_kdump_vmcore(s, errp);
> +} else {
> +create_vmcore(s, errp);
> +}
> +}
> +
> +static void *dump_thread(void *data)
> +{
> +GlobalDumpState *global = (GlobalDumpState *)data;
> +dump_process(global->gds_cur, NULL);
> +dump_state_release(global);
> +return NULL;
> +}
> +
>  void qmp_dump_guest_memory(bool paging, const char *file,
> bool has_detach, bool detach,
> bool 

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Peter Xu]

On 11/27/2015 01:14 PM, Fam Zheng wrote:
> On Fri, 11/27 10:48, Peter Xu wrote:
>> This patch implements "detach" for "dump-guest-memory" command. When
>> specified, one background thread is created to do the dump work.
>>
>> When there is a dump running in background, the commands "stop",
>> "cont" and "dump-guest-memory" will fail directly, with a hint
>> telling user: "there is a dump in progress".
>>
>> Although it is not quite essential for now, the new codes are trying
>> to make sure the dump is thread safe. To do this, one list is added
>> into GuestPhysBlockList to track all the referenced MemoryRegions
>> during dump process. We should make sure each MemoryRegion would
>> only be referenced for once.
>>
>> One global struct "GlobalDumpState" is added to store some global
>> informations for dump procedures. One mutex is used to protect the
>> global dump info.
> 
> This patch doesn't handle the incoming migration case, i.e. when QEMU is
> started with "-incoming", or "-incoming defer". Dump can go wrong when 
> incoming
> migration happens at the same time.
> 
>>
>> Signed-off-by: Peter Xu 
>> ---
>>  dump.c  | 114 
>> +---
>>  include/qemu-common.h   |   4 ++
>>  include/sysemu/dump.h   |  10 
>>  include/sysemu/memory_mapping.h |   8 +++
>>  memory_mapping.c|  46 +++-
>>  qmp.c   |  14 +
>>  6 files changed, 188 insertions(+), 8 deletions(-)
>>
>> diff --git a/dump.c b/dump.c
>> index d79e0ed..dfd56aa 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -73,6 +73,7 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>>  static int dump_cleanup(DumpState *s)
>>  {
>>  guest_phys_blocks_free(>guest_phys_blocks);
>> +guest_memory_region_free(>guest_phys_blocks);
>>  memory_mapping_list_free(>list);
>>  close(s->fd);
>>  if (s->resume) {
>> @@ -1427,6 +1428,9 @@ static void dump_init(DumpState *s, int fd, bool 
>> has_format,
>>  Error *err = NULL;
>>  int ret;
>>  
>> +s->has_format = has_format;
>> +s->format = format;
>> +
>>  /* kdump-compressed is conflict with paging and filter */
>>  if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>>  assert(!paging && !has_filter);
>> @@ -1580,6 +1584,86 @@ cleanup:
>>  dump_cleanup(s);
>>  }
>>  
>> +extern GlobalDumpState global_dump_state;
> 
> dump_state_get_global() returns a pointer to the local static variable, why do
> you need this?
> 
> But what I really wonder is why a single boolean is not enough: the DumpState
> pointer can be passed to dump_thread, so it doesn't need to be global, then 
> you
> don't need the mutex.
> 
>> +
>> +static GlobalDumpState *dump_state_get_global(void)
>> +{
>> +static bool init = false;
>> +static GlobalDumpState global_dump_state;
>> +if (unlikely(!init)) {
>> +qemu_mutex_init(_dump_state.gds_mutex);
>> +global_dump_state.gds_cur = NULL;
>> +init = true;
>> +}
>> +return _dump_state;
>> +}
>> +
>> +/* Returns non-zero if there is existing dump in progress, otherwise
>> + * zero is returned. */
>> +bool dump_in_progress(void)
>> +{
>> +GlobalDumpState *global = dump_state_get_global();
>> +/* we do not need to take the mutex here if we are checking the
>> + * pointer only. */
>> +return (!!global->gds_cur);
>> +}
>> +
>> +/* Trying to create one DumpState. This will fail if there is an
>> + * existing one. Return DumpState pointer if succeeded, otherwise
>> + * NULL is returned. */
>> +static DumpState *dump_state_create(GlobalDumpState *global)
>> +{
>> +DumpState *cur = NULL;
>> +qemu_mutex_lock(>gds_mutex);
>> +
>> +cur = global->gds_cur;
>> +if (cur) {
>> +/* one dump in progress */
>> +cur = NULL;
>> +} else {
>> +/* we are the first! */
>> +cur = g_malloc0(sizeof(*cur));
>> +global->gds_cur = cur;
>> +}
>> +
>> +qemu_mutex_unlock(>gds_mutex);
>> +return cur;
>> +}
>> +
>> +/* release current DumpState structure */
>> +static void dump_state_release(GlobalDumpState *global)
>> +{
>> +DumpState *cur = NULL;
>> +qemu_mutex_lock(>gds_mutex);
>> +
>> +cur = global->gds_cur;
>> +/* we should never call release before having one */
>> +assert(cur);
>> +global->gds_cur = NULL;
>> +
>> +qemu_mutex_unlock(>gds_mutex);
>> +dump_cleanup(cur);
>> +g_free(cur);
>> +}
>> +
>> +/* this operation might be time consuming. */
>> +static void dump_process(DumpState *s, Error **errp)
>> +{
>> +if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>> +create_kdump_vmcore(s, errp);
>> +} else {
>> +create_vmcore(s, errp);
>> +}
>> +}
>> +
>> +static void *dump_thread(void *data)
>> +{
>> +GlobalDumpState *global = (GlobalDumpState *)data;
>> +dump_process(global->gds_cur, NULL);
>> +

Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Fam Zheng]

On Fri, 11/27 13:14, Fam Zheng wrote:
> But what I really wonder is why a single boolean is not enough: the DumpState
> pointer can be passed to dump_thread, so it doesn't need to be global, then 
> you
> don't need the mutex.

Never mind, it's useful in later patches.

Fam