From mboxrd@z Thu Jan  1 00:00:00 1970
From: Keir Fraser <keir@xen.org>
Subject: Re: [PATCH 2 of 3] RFC: mem_event: use wait queue when
 ring is full
Date: Wed, 23 Nov 2011 09:01:19 +0000
Message-ID: <CAF266DF.3489F%keir@xen.org>
References: <4ECCC1970200007800062875@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xensource.com>
In-Reply-To: <4ECCC1970200007800062875@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xensource.com/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xensource.com>
List-Help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-Subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
Sender: xen-devel-bounces@lists.xensource.com
Errors-To: xen-devel-bounces@lists.xensource.com
To: Jan Beulich <JBeulich@suse.com>, Olaf Hering <olaf@aepfle.de>
Cc: xen-devel@lists.xensource.com
List-Id: xen-devel@lists.xenproject.org

On 23/11/2011 08:49, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 22.11.11 at 22:13, Olaf Hering <olaf@aepfle.de> wrote:
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -14,6 +14,7 @@
>>  #include <xen/nodemask.h>
>>  #include <xen/radix-tree.h>
>>  #include <xen/multicall.h>
>> +#include <xen/wait.h>
>>  #include <public/xen.h>
>>  #include <public/domctl.h>
>>  #include <public/sysctl.h>
>> @@ -192,6 +193,10 @@ struct mem_event_domain
>>      mem_event_front_ring_t front_ring;
>>      /* event channel port (vcpu0 only) */
>>      int xen_port;
>> +    /* mem_event bit for vcpu->pause_flags */
>> +    int mem_event_bit;
> 
> Perhaps pause_bit would be a better name here? Or at least, as for
> the first patch, the mem_ prefix should go away (or really the
> mem_event_ one, but that would just leave "bit", which is how I got
> to the above proposal).

Yes, mem_event_bit is a lazy name here. Doesn't really describe what the bit
is actually for. It's obviously mem_event related because of the struct it
is a member of.

>> +    /* list of vcpus waiting for room in the ring */
>> +    struct waitqueue_head wq;
>>  };
>>  
>>  struct mem_event_per_domain
>> @@ -615,9 +620,12 @@ static inline struct domain *next_domain
>>   /* VCPU affinity has changed: migrating to a new CPU. */
>>  #define _VPF_migrating       3
>>  #define VPF_migrating        (1UL<<_VPF_migrating)
>> - /* VCPU is blocked on memory-event ring. */
>> -#define _VPF_mem_event       4
>> -#define VPF_mem_event        (1UL<<_VPF_mem_event)
>> + /* VCPU is blocked on mem_paging ring. */
>> +#define _VPF_me_mem_paging   4
>> +#define VPF_me_mem_paging    (1UL<<_VPF_me_mem_paging)
>> + /* VCPU is blocked on mem_access ring. */
>> +#define _VPF_me_mem_access   5
>> +#define VPF_me_mem_access    (1UL<<_VPF_me_mem_access)
> 
> Same here - the mem_ seems superfluous.

Mem_event-related flags in a more general grouping do require a mem_ prefix
imo. The names need to stand on their own and still be descriptive.

 -- Keir

> Jan
> 
>>  
>>  static inline int vcpu_runnable(struct vcpu *v)
>>  {
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel