These patches are similar to changeset 10747 (which added vcpu_mark_events_pending), in that they allow PPC to work around some unusually-sized atomic operations, this time in the grant table driver. Keir, do you think it''s a good idea to make a wiki page to list where the current ABI is causing problems? That way, when the ABI opens up again (Xen 4?), we''ll have a list of places to fix? Anyways, I expect you''ll have alternate name/interface suggestions in the patches, so they''re just RFC for now. Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com> diff -r b3dfb02fc2c2 xen/common/grant_table.c --- a/xen/common/grant_table.c Thu Jun 22 10:58:41 2006 -0500 +++ b/xen/common/grant_table.c Fri Jun 23 16:13:48 2006 -0500 @@ -287,10 +287,10 @@ __gnttab_map_grant_ref( if ( !(op->flags & GNTMAP_readonly) && !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) ) - clear_bit(_GTF_writing, &sha->flags); + clear_entry_flag(_GTF_writing, &sha->flags); if ( !act->pin ) - clear_bit(_GTF_reading, &sha->flags); + clear_entry_flag(_GTF_reading, &sha->flags); unlock_out: spin_unlock(&rd->grant_table->lock); @@ -425,10 +425,10 @@ __gnttab_unmap_grant_ref( if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) && !(flags & GNTMAP_readonly) ) - clear_bit(_GTF_writing, &sha->flags); + clear_entry_flag(_GTF_writing, &sha->flags); if ( act->pin == 0 ) - clear_bit(_GTF_reading, &sha->flags); + clear_entry_flag(_GTF_reading, &sha->flags); unmap_out: op->status = rc; @@ -889,11 +889,11 @@ gnttab_release_mappings( } if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 ) - clear_bit(_GTF_writing, &sha->flags); + clear_entry_flag(_GTF_writing, &sha->flags); } if ( act->pin == 0 ) - clear_bit(_GTF_reading, &sha->flags); + clear_entry_flag(_GTF_reading, &sha->flags); spin_unlock(&rd->grant_table->lock); diff -r b3dfb02fc2c2 xen/include/asm-ia64/grant_table.h --- a/xen/include/asm-ia64/grant_table.h Thu Jun 22 10:58:41 2006 -0500 +++ b/xen/include/asm-ia64/grant_table.h Fri Jun 23 16:13:48 2006 -0500 @@ -55,4 +55,9 @@ void guest_physmap_add_page(struct domai #define gnttab_log_dirty(d, f) ((void)0) +static inline void clear_entry_flag(unsigned long nr, uint16_t *addr) +{ + clear_bit(nr, addr); +} + #endif /* __ASM_GRANT_TABLE_H__ */ diff -r b3dfb02fc2c2 xen/include/asm-x86/grant_table.h --- a/xen/include/asm-x86/grant_table.h Thu Jun 22 10:58:41 2006 -0500 +++ b/xen/include/asm-x86/grant_table.h Fri Jun 23 16:13:48 2006 -0500 @@ -33,4 +33,9 @@ int destroy_grant_host_mapping( #define gnttab_log_dirty(d, f) mark_dirty((d), (f)) +static inline void clear_entry_flag(unsigned long nr, uint16_t *addr) +{ + clear_bit(nr, addr); +} + #endif /* __ASM_GRANT_TABLE_H__ */ For reference, here''s the Xen PPC hack: diff -r b3dfb02fc2c2 xen/include/asm-ppc/grant_table.h --- a/xen/include/asm-ppc/grant_table.h Thu Jun 22 10:58:41 2006 -0500 +++ b/xen/include/asm-ppc/grant_table.h Fri Jun 23 16:13:48 2006 -0500 @@ -26,4 +26,16 @@ #define mark_dirty(d, f) ((void )0) #include "../asm-x86/grant_table.h" +static inline void clear_entry_flag(unsigned long nr, uint16_t *addr) +{ + unsigned long *laddr; + unsigned long lnr; + + BUG_ON((ulong)addr % sizeof(ulong)); + + lnr = (BITS_PER_LONG - (sizeof(*addr) * 8)) + nr; + laddr = (unsigned long *)addr; + clear_bit(lnr, laddr); +} + #endif /* __ASM_PPC_GRANT_TABLE_H__ */ The Linux patch applies to the current sparse tree with slight offsets: diff -r 6f3d44537b76 drivers/xen/core/gnttab.c --- a/drivers/xen/core/gnttab.c Fri Jun 16 16:07:38 2006 -0500 +++ b/drivers/xen/core/gnttab.c Fri Jun 23 16:14:27 2006 -0500 @@ -201,7 +201,7 @@ gnttab_end_foreign_access_ref(grant_ref_ printk(KERN_ALERT "WARNING: g.e. still in use!\n"); return 0; } - } while ((nflags = synch_cmpxchg(&shared[ref].flags, flags, 0)) !+ } while ((nflags = gnttab_cmpxchg_flags(&shared[ref].flags, flags, 0)) ! flags); return 1; @@ -256,7 +256,7 @@ gnttab_end_foreign_transfer_ref(grant_re * reference and return failure (== 0). */ while (!((flags = shared[ref].flags) & GTF_transfer_committed)) { - if (synch_cmpxchg(&shared[ref].flags, flags, 0) == flags) + if (gnttab_cmpxchg_flags(&shared[ref].flags, flags, 0) == flags) return 0; cpu_relax(); } diff -r 6f3d44537b76 include/asm-i386/mach-xen/asm/synch_bitops.h --- a/include/asm-i386/mach-xen/asm/synch_bitops.h Fri Jun 16 16:07:38 2006 -0500 +++ b/include/asm-i386/mach-xen/asm/synch_bitops.h Fri Jun 23 16:14:27 2006 -0500 @@ -138,4 +138,6 @@ static __inline__ int synch_var_test_bit synch_const_test_bit((nr),(addr)) : \ synch_var_test_bit((nr),(addr))) +#define gnttab_cmpxchg_flags synch_cmpxchg + #endif /* __XEN_SYNCH_BITOPS_H__ */ diff -r 6f3d44537b76 include/asm-ia64/synch_bitops.h --- a/include/asm-ia64/synch_bitops.h Fri Jun 16 16:07:38 2006 -0500 +++ b/include/asm-ia64/synch_bitops.h Fri Jun 23 16:14:27 2006 -0500 @@ -58,4 +58,6 @@ static __inline__ int synch_var_test_bit synch_const_test_bit((nr),(addr)) : \ synch_var_test_bit((nr),(addr))) +#define gnttab_cmpxchg_flags synch_cmpxchg + #endif /* __XEN_SYNCH_BITOPS_H__ */ For reference, the Linux PPC hack: diff -r 6f3d44537b76 include/asm-powerpc/xen/asm/synch_bitops.h --- a/include/asm-powerpc/xen/asm/synch_bitops.h Fri Jun 16 16:07:38 2006 -0500 +++ b/include/asm-powerpc/xen/asm/synch_bitops.h Fri Jun 23 16:14:27 2006 -0500 @@ -21,4 +21,28 @@ #error "this only works for CONFIG_SMP" #endif +/* HACK: grant_entry.flags is two bytes, but our atomic instructions + * only store in 4/8 byte quantities. However, because it''s part of the guest + * ABI, we can''t change its size without breaking backwards compatibility. In + * this particular case, struct grant_entry is big enough that we can safely + * store 4 bytes into it. However, some munging is needed... + */ +static inline __u16 gnttab_cmpxchg_flags(__u16 *ptr, __u16 o, __u16 n) +{ + volatile __u32 *laddr; + unsigned long shift = (sizeof(*laddr) - sizeof(*ptr)) * 8; + __u32 orig; + __u32 old; + __u32 new; + + BUG_ON((long)ptr % sizeof(*laddr)); + + laddr = (volatile __u32 *)ptr; + orig = *laddr; + old = ((unsigned long)o << shift) | (orig & ((1UL<<shift)-1)); + new = ((unsigned long)n << shift) | (orig & ((1UL<<shift)-1)); + + return synch_cmpxchg(laddr, old, new); +} + #endif /* __SYNCH_BITOPS_H__ */ -- Hollis Blanchard IBM Linux Technology Center _______________________________________________ Xen-ppc-devel mailing list Xen-ppc-devel@lists.xensource.com http://lists.xensource.com/xen-ppc-devel
Keir Fraser
2006-Jun-27 10:39 UTC
[Xen-devel] Re: [rfc] [patch] grant_entry.flags accessors
On 23 Jun 2006, at 22:19, Hollis Blanchard wrote:> These patches are similar to changeset 10747 (which added > vcpu_mark_events_pending), in that they allow PPC to work around some > unusually-sized atomic operations, this time in the grant table driver. > > Keir, do you think it''s a good idea to make a wiki page to list where > the current ABI is causing problems? That way, when the ABI opens up > again (Xen 4?), we''ll have a list of places to fix? > > Anyways, I expect you''ll have alternate name/interface suggestions in > the patches, so they''re just RFC for now.You should be able to hide all this behind the generic atomic operations without slowing down sufficiently aligned operations at all. e.g. something like: define clear_bit(p,i) ((alignof(p)>=alignof(long)) ? clear_bit(p,i) : clear_bit_unaligned(p,i)) -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hollis Blanchard
2006-Jun-27 15:06 UTC
[Xen-devel] Re: [rfc] [patch] grant_entry.flags accessors
On Tue, 2006-06-27 at 11:39 +0100, Keir Fraser wrote:> On 23 Jun 2006, at 22:19, Hollis Blanchard wrote: > > > These patches are similar to changeset 10747 (which added > > vcpu_mark_events_pending), in that they allow PPC to work around some > > unusually-sized atomic operations, this time in the grant table driver. > > > > Keir, do you think it''s a good idea to make a wiki page to list where > > the current ABI is causing problems? That way, when the ABI opens up > > again (Xen 4?), we''ll have a list of places to fix? > > > > Anyways, I expect you''ll have alternate name/interface suggestions in > > the patches, so they''re just RFC for now. > > You should be able to hide all this behind the generic atomic > operations without slowing down sufficiently aligned operations at all. > > e.g. something like: > define clear_bit(p,i) ((alignof(p)>=alignof(long)) ? clear_bit(p,i) : > clear_bit_unaligned(p,i))In the couple cases so far, we know that even though the field is only one or two bytes, it''s actually safe to do a four-byte load/store to it because the containing structure is large enough. I''m not really comfortable with making that a blanket assumption. I''d really like to know exactly what we''re overwriting when performing these hacks^Woperations. -- Hollis Blanchard IBM Linux Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2006-Jun-27 15:24 UTC
[Xen-devel] Re: [rfc] [patch] grant_entry.flags accessors
On 27 Jun 2006, at 16:06, Hollis Blanchard wrote:> In the couple cases so far, we know that even though the field is only > one or two bytes, it''s actually safe to do a four-byte load/store to it > because the containing structure is large enough. > > I''m not really comfortable with making that a blanket assumption. I''d > really like to know exactly what we''re overwriting when performing > these > hacks^Woperations.You won''t be overwriting anything else since you''ll be performing an atomic read-modify-write that does not change any of the bits outside the subword of interest. More precisely, you may overwrite those bits outside the subword, but it''s guaranteed to be with the current contents so there''ll be no externally visible change and you won''t clobber anything. Since you''ll do an aligned longword operation you can''t end up straddling a page boundary or anything nasty like that. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hollis Blanchard
2006-Jun-27 16:11 UTC
[Xen-devel] Re: [rfc] [patch] grant_entry.flags accessors
On Tue, 2006-06-27 at 16:24 +0100, Keir Fraser wrote:> On 27 Jun 2006, at 16:06, Hollis Blanchard wrote: > > > In the couple cases so far, we know that even though the field is only > > one or two bytes, it''s actually safe to do a four-byte load/store to it > > because the containing structure is large enough. > > > > I''m not really comfortable with making that a blanket assumption. I''d > > really like to know exactly what we''re overwriting when performing > > these > > hacks^Woperations. > > You won''t be overwriting anything else since you''ll be performing an > atomic read-modify-write that does not change any of the bits outside > the subword of interest. More precisely, you may overwrite those bits > outside the subword, but it''s guaranteed to be with the current > contents so there''ll be no externally visible change and you won''t > clobber anything.Right, I''m not worried about clobbering data. I am a little worried about false conflicts in the reservations. Atomics on PPC, and many other processors, are implemented as "load with reservation; conditional store", where conditional store fails if another processor conflicted with the reservation. I think typically a processor only has a single reservation. Practically speaking this probably isn''t likely to hit us, but the whole thing is really making me uncomfortable.> Since you''ll do an aligned longword operation you > can''t end up straddling a page boundary or anything nasty like that.OK, that''s a good point. In general I''m skeptical of blindly performing these operations on unknown data. I think it''s pretty clear that is not a good interface, and what you''re suggesting is codifying this behavior in the interface. More practically speaking, I would be shocked if the Linux maintainers would accept that sort of change to their atomic interfaces. -- Hollis Blanchard IBM Linux Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2006-Jun-27 16:55 UTC
[Xen-devel] Re: [rfc] [patch] grant_entry.flags accessors
On 27 Jun 2006, at 17:11, Hollis Blanchard wrote:> In general I''m skeptical of blindly performing these operations on > unknown data. I think it''s pretty clear that is not a good interface, > and what you''re suggesting is codifying this behavior in the interface. > > More practically speaking, I would be shocked if the Linux maintainers > would accept that sort of change to their atomic interfaces.I shouldn''t imagine they''d mind the interface being made more permissive, with no performance loss for the common case and no extra API complexity, but anyway: How about add a ARCH_SUPPORTS_UNALIGNED_CMPXCHG and move special gnttab_cmpxchg() definition to gnttab.c, compilation dependent on that? Or rename the gnttab_cmpxchg to synch_cmpxchg_unaligned so at least the name is a bit more generic. It could then be used in other contexts. The Xen change is okay, but gnttab_clear_flag() would be a better function name if the function is really going to be gnttab specific. Or rename to clear_bit_unaligned and move definition to bitops.h. As with synch_cmpxchg_unaligned, the new function could then be used in other contexts. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jimi Xenidis
2006-Jun-27 17:45 UTC
Re: [Xen-devel] Re: [rfc] [patch] grant_entry.flags accessors
On Jun 27, 2006, at 12:55 PM, Keir Fraser wrote:> I shouldn''t imagine they''d mind the interface being made more > permissive, with no performance loss for the common case and no > extra API complexityHmm, the interesting part is that as far as bit-ops go in Linux x86 converged to longs rather then ppc converging to some the arbitrary bit method: see: include/asm-i386/bitops.h clear_bit 71 static inline void clear_bit (int nr, volatile unsigned long * addr) I''ve been told this was to solve a performance issue, but I am no expert.> , but anyway: How about add a ARCH_SUPPORTS_UNALIGNED_CMPXCHG and > move special gnttab_cmpxchg() definition to gnttab.c, compilation > dependent on that?cmpxchg will take 1,2,4 byte, and pc will take to, however we need to abstract these anyway because the values are castes to a u32 and or''d in an little endian way.> Or rename the gnttab_cmpxchg to synch_cmpxchg_unaligned so at least > the name is a bit more generic. It could then be used in other > contexts.The _main_ issue is not really alignof(p), beacuse the Xen code is careful about that, but sizeof(*p)which we need to calculate the bit position. Thats where this interface can get a little loosy for me. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2006-Jun-28 06:43 UTC
Re: [Xen-devel] Re: [rfc] [patch] grant_entry.flags accessors
On 27 Jun 2006, at 18:45, Jimi Xenidis wrote:> Hmm, the interesting part is that as far as bit-ops go in Linux x86 > converged to longs rather then ppc converging to some the arbitrary > bit method: > see: > include/asm-i386/bitops.h clear_bit 71 static inline void > clear_bit(int nr, volatile unsigned long * addr) > > I''ve been told this was to solve a performance issue, but I am no > expert.Well there would be a performance impact for other architectures, no doubt. x86/64 never moved to longs for bitops.>> , but anyway: How about add a ARCH_SUPPORTS_UNALIGNED_CMPXCHG and >> move special gnttab_cmpxchg() definition to gnttab.c, compilation >> dependent on that? > > cmpxchg will take 1,2,4 byte, and pc will take to, however we need to > abstract these anyway because the values are castes to a u32 and or''d > in an little endian way. > >> Or rename the gnttab_cmpxchg to synch_cmpxchg_unaligned so at least >> the name is a bit more generic. It could then be used in other >> contexts. > > The _main_ issue is not really alignof(p), beacuse the Xen code is > careful about that, but sizeof(*p)which we need to calculate the bit > position. Thats where this interface can get a little loosy for me.Then pick a different name. :-) *_subword()? I won''t put a gnttab_foo() in synch_bitops.h. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hollis Blanchard
2006-Jun-28 17:07 UTC
Re: [Xen-devel] Re: [rfc] [patch] grant_entry.flags accessors
On Wed, 2006-06-28 at 07:43 +0100, Keir Fraser wrote:> On 27 Jun 2006, at 18:45, Jimi Xenidis wrote: > > > Hmm, the interesting part is that as far as bit-ops go in Linux x86 > > converged to longs rather then ppc converging to some the arbitrary > > bit method: > > see: > > include/asm-i386/bitops.h clear_bit 71 static inline void > > clear_bit(int nr, volatile unsigned long * addr) > > > > I''ve been told this was to solve a performance issue, but I am no > > expert. > > Well there would be a performance impact for other architectures, no > doubt. x86/64 never moved to longs for bitops.They wouldn''t have to. Because other architectures did, especially i386, that ensures that all common code will use bitops only on longs, which is what we''d like to see in Xen as well. -- Hollis Blanchard IBM Linux Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Keir, here is the resend based on your feedback. The Linux patch applies with small offsets to the sparse tree. The Xen patch follows that. Use explicit accessors to handle unusually-sized atomic operations in grant table code. Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com> diff -r 6f3d44537b76 drivers/xen/core/gnttab.c --- a/drivers/xen/core/gnttab.c Fri Jun 16 16:07:38 2006 -0500 +++ b/drivers/xen/core/gnttab.c Fri Jun 23 15:57:39 2006 -0500 @@ -201,7 +201,7 @@ gnttab_end_foreign_access_ref(grant_ref_ printk(KERN_ALERT "WARNING: g.e. still in use!\n"); return 0; } - } while ((nflags = synch_cmpxchg(&shared[ref].flags, flags, 0)) !+ } while ((nflags = synch_cmpxchg_subword(&shared[ref].flags, flags, 0)) ! flags); return 1; @@ -256,7 +256,7 @@ gnttab_end_foreign_transfer_ref(grant_re * reference and return failure (== 0). */ while (!((flags = shared[ref].flags) & GTF_transfer_committed)) { - if (synch_cmpxchg(&shared[ref].flags, flags, 0) == flags) + if (synch_cmpxchg_subword(&shared[ref].flags, flags, 0) == flags) return 0; cpu_relax(); } diff -r 6f3d44537b76 include/asm-i386/mach-xen/asm/synch_bitops.h --- a/include/asm-i386/mach-xen/asm/synch_bitops.h Fri Jun 16 16:07:38 2006 -0500 +++ b/include/asm-i386/mach-xen/asm/synch_bitops.h Fri Jun 23 15:57:39 2006 -0500 @@ -138,4 +138,6 @@ static __inline__ int synch_var_test_bit synch_const_test_bit((nr),(addr)) : \ synch_var_test_bit((nr),(addr))) +#define synch_cmpxchg_subword synch_cmpxchg + #endif /* __XEN_SYNCH_BITOPS_H__ */ diff -r 6f3d44537b76 include/asm-ia64/synch_bitops.h --- a/include/asm-ia64/synch_bitops.h Fri Jun 16 16:07:38 2006 -0500 +++ b/include/asm-ia64/synch_bitops.h Fri Jun 23 15:57:39 2006 -0500 @@ -58,4 +58,6 @@ static __inline__ int synch_var_test_bit synch_const_test_bit((nr),(addr)) : \ synch_var_test_bit((nr),(addr))) +#define synch_cmpxchg_subword synch_cmpxchg + #endif /* __XEN_SYNCH_BITOPS_H__ */ The Xen patch: diff -r e06866a6e2b7 xen/common/grant_table.c --- a/xen/common/grant_table.c Thu Jun 29 13:10:42 2006 -0400 +++ b/xen/common/grant_table.c Thu Jun 29 15:27:06 2006 -0500 @@ -287,10 +287,10 @@ __gnttab_map_grant_ref( if ( !(op->flags & GNTMAP_readonly) && !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) ) - clear_bit(_GTF_writing, &sha->flags); + gnttab_clear_flag(_GTF_writing, &sha->flags); if ( !act->pin ) - clear_bit(_GTF_reading, &sha->flags); + gnttab_clear_flag(_GTF_reading, &sha->flags); unlock_out: spin_unlock(&rd->grant_table->lock); @@ -425,10 +425,10 @@ __gnttab_unmap_grant_ref( if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) && !(flags & GNTMAP_readonly) ) - clear_bit(_GTF_writing, &sha->flags); + gnttab_clear_flag(_GTF_writing, &sha->flags); if ( act->pin == 0 ) - clear_bit(_GTF_reading, &sha->flags); + gnttab_clear_flag(_GTF_reading, &sha->flags); unmap_out: op->status = rc; @@ -889,11 +889,11 @@ gnttab_release_mappings( } if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 ) - clear_bit(_GTF_writing, &sha->flags); + gnttab_clear_flag(_GTF_writing, &sha->flags); } if ( act->pin == 0 ) - clear_bit(_GTF_reading, &sha->flags); + gnttab_clear_flag(_GTF_reading, &sha->flags); spin_unlock(&rd->grant_table->lock); diff -r e06866a6e2b7 xen/include/asm-ia64/grant_table.h --- a/xen/include/asm-ia64/grant_table.h Thu Jun 29 13:10:42 2006 -0400 +++ b/xen/include/asm-ia64/grant_table.h Thu Jun 29 15:27:06 2006 -0500 @@ -55,4 +55,9 @@ void guest_physmap_add_page(struct domai #define gnttab_log_dirty(d, f) ((void)0) +static inline void gnttab_clear_flag(unsigned long nr, uint16_t *addr) +{ + clear_bit(nr, addr); +} + #endif /* __ASM_GRANT_TABLE_H__ */ diff -r e06866a6e2b7 xen/include/asm-x86/grant_table.h --- a/xen/include/asm-x86/grant_table.h Thu Jun 29 13:10:42 2006 -0400 +++ b/xen/include/asm-x86/grant_table.h Thu Jun 29 15:27:06 2006 -0500 @@ -33,4 +33,9 @@ int destroy_grant_host_mapping( #define gnttab_log_dirty(d, f) mark_dirty((d), (f)) +static inline void gnttab_clear_flag(unsigned long nr, uint16_t *addr) +{ + clear_bit(nr, addr); +} + #endif /* __ASM_GRANT_TABLE_H__ */ -- Hollis Blanchard IBM Linux Technology Center _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel