Re: [Xen-changelog] [xen-unstable] xen: Split domain_flags into discrete first-class fields in the

Re: [Xen-changelog] [xen-unstable] xen: Split domain_flags into discrete first-class fields in the

[Hollis Blanchard]

On Fri, 2007-03-30 at 16:10 -0700, Xen patchbot-unstable wrote:
> # HG changeset patch
> # User kfraser@localhost.localdomain
> # Date 1175177666 -3600
> # Node ID 4b13fc910acf0019c27cbae35181433b381e88d1
> # Parent  31f20aaac8188bc1366b80e55e47b328db425180
> xen: Split domain_flags into discrete first-class fields in the
> domain structure. This makes them quicker to access, and simplifies
> domain pause and checking of runnable status.

> diff -r 31f20aaac818 -r 4b13fc910acf xen/common/domain.c
> --- a/xen/common/domain.c	Thu Mar 29 13:29:24 2007 +0100
> +++ b/xen/common/domain.c	Thu Mar 29 15:14:26 2007 +0100
> @@ -262,8 +264,12 @@ void domain_kill(struct domain *d)
>  {
>      domain_pause(d);
> 
> -    if ( test_and_set_bit(_DOMF_dying, &d->domain_flags) )
> +    /* Already dying? Then bail. */
> +    if ( xchg(&d->is_dying, 1) )
>          return;
> +
> +    /* Tear down state /after/ setting the dying flag. */
> +    smp_wmb();
> 
>      gnttab_release_mappings(d);
>      domain_relinquish_resources(d);

You're now doing xchg() on a 1-byte variable, which does not work on
PowerPC.

This is an interface problem: using the interface in a way that works on
x86 fails on other architectures. PLEASE let's redefine the interface to
prevent this from happening. In this case, that means replacing the
xchg() macro with
        static inline xchg(atomic_t *ptr, atomic_t val)
and changing the type of 'is_dying'.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Re: [Xen-changelog] [xen-unstable] xen: Split domain_flags into discrete first-class fields in the

[Keir Fraser]

On 5/4/07 16:44, "Hollis Blanchard" <hollisb@us.ibm.com> wrote:

> This is an interface problem: using the interface in a way that works on
> x86 fails on other architectures. PLEASE let's redefine the interface to
> prevent this from happening. In this case, that means replacing the
> xchg() macro with
>         static inline xchg(atomic_t *ptr, atomic_t val)
> and changing the type of 'is_dying'.

Just need to define bool_t appropriately. What do you need: a long?

 -- Keir

Re: Re: [Xen-changelog] [xen-unstable] xen: Split domain_flags into discrete first-class fields in the

[Keir Fraser]

On 5/4/07 16:56, "Keir Fraser" <keir@xensource.com> wrote:

> On 5/4/07 16:44, "Hollis Blanchard" <hollisb@us.ibm.com> wrote:
> 
>> This is an interface problem: using the interface in a way that works on
>> x86 fails on other architectures. PLEASE let's redefine the interface to
>> prevent this from happening. In this case, that means replacing the
>> xchg() macro with
>>         static inline xchg(atomic_t *ptr, atomic_t val)
>> and changing the type of 'is_dying'.
> 
> Just need to define bool_t appropriately. What do you need: a long?

Does PowerPC support atomic byte loads and stores by the way (i.e.,
concurrent loads and stores to adjacent bytes by different processors do not
conflict with each other)? In which case it might be worth keeping bool_t
and defining atomic_bool_t or atomic_rmw_bool_t for bools that need to be
atomically read-modified-written. That would avoid bloating critical
structures for the few bools that need atomic r-m-w semantics.

 -- Keir

Re: Re: [Xen-changelog] [xen-unstable] xen: Split domain_flags into discrete first-class fields in the

[Hollis Blanchard]

On Thu, 2007-04-05 at 17:59 +0100, Keir Fraser wrote:
> On 5/4/07 16:56, "Keir Fraser" <keir@xensource.com> wrote:
> 
> > On 5/4/07 16:44, "Hollis Blanchard" <hollisb@us.ibm.com> wrote:
> > 
> >> This is an interface problem: using the interface in a way that works on
> >> x86 fails on other architectures. PLEASE let's redefine the interface to
> >> prevent this from happening. In this case, that means replacing the
> >> xchg() macro with
> >>         static inline xchg(atomic_t *ptr, atomic_t val)
> >> and changing the type of 'is_dying'.
> > 
> > Just need to define bool_t appropriately. What do you need: a long?
> 
> Does PowerPC support atomic byte loads and stores by the way (i.e.,
> concurrent loads and stores to adjacent bytes by different processors do not
> conflict with each other)?

Yes, there are single byte load and store instructions.

> In which case it might be worth keeping bool_t
> and defining atomic_bool_t or atomic_rmw_bool_t for bools that need to be
> atomically read-modified-written. That would avoid bloating critical
> structures for the few bools that need atomic r-m-w semantics.

If that's your preference.

However, as long as xchg() accepts all pointer types, this problem will
reoccur. We've had the same problem with the set_bit() interface in the
past, and I see x86 still uses a void* as the pointer argument there.
x86 Linux doesn't use void* for bitops and it's for exactly this reason.

These are not difficult changes to make, and solve real long-term
maintenance problems. I'm sure if x86 had this issue, an arch-neutral
API would have been in place from day one.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Re: [Xen-changelog] [xen-unstable] xen: Split domain_flags into discrete first-class fields in the

[Hollis Blanchard]

On Thu, 2007-04-05 at 16:56 +0100, Keir Fraser wrote:
> On 5/4/07 16:44, "Hollis Blanchard" <hollisb@us.ibm.com> wrote:
> 
> > This is an interface problem: using the interface in a way that works on
> > x86 fails on other architectures. PLEASE let's redefine the interface to
> > prevent this from happening. In this case, that means replacing the
> > xchg() macro with
> >         static inline xchg(atomic_t *ptr, atomic_t val)
> > and changing the type of 'is_dying'.
> 
> Just need to define bool_t appropriately. What do you need: a long?

An int would be fine, and that would solve today's problem.

How will that prevent the same problem from recurring in the future?

-- 
Hollis Blanchard
IBM Linux Technology Center