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

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel