On Tue, Dec 19, 2017 at 02:01:59PM +0100, Michal Hocko wrote: > On Tue 19-12-17 04:46:05, Matthew Wilcox wrote: > > On Tue, Dec 19, 2017 at 09:07:31AM +0100, Michal Hocko wrote: > > > On Sat 16-12-17 08:44:22, Matthew Wilcox wrote: > > > > From: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx> > > > > > > > > Instead of putting the ifdef in the middle of the definition of struct > > > > page, pull it forward to the rest of the ifdeffery around the SLUB > > > > cmpxchg_double optimisation. > > > > > > > > Signed-off-by: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx> > > > > > > The definition of struct page looks better now. I think that slub.c > > > needs some love as well. I haven't checked too deeply but it seems that > > > it assumes counters to be unsigned long in some places. Maybe I've > > > missed some ifdef-ery but using the native type would be much better > > > > I may have missed something, but I checked its use of 'counters' while > > I was working on this patch, and I didn't *see* a problem. > > I didn't check too closely but I can see code like this in slub.c > static inline void set_page_slub_counters(struct page *page, unsigned long counters_new) > resp. > static inline bool cmpxchg_double_slab(struct kmem_cache *s, struct page *page, > void *freelist_old, unsigned long counters_old, > void *freelist_new, unsigned long counters_new, > const char *n) > > which always uses unsigned long for the value rather than unsigned int. > But maybe those paths are never dealing with an out-of-scope value. > Using your new type there would cleanup that thing a bit. OK, here's how I read the code in slub. Christoph, please let me know if I misunderstand. slub wants to atomically update both freelist and its counters, so it has 96 bits of information to update atomically (on 64 bit), or 64 bits on 32-bit machines. We don't have a 96-bit atomic-cmpxchg, but we do have a 128-bit atomic-cmpxchg on some architectures. So _if_ we're going to use cmpxchg_double(), then we need counters to be an unsigned long. If we're not then counters needs to be an unsigned int so it doesn't overlap with _refcount, which is not going to be protected by slab_lock. Now I look at it some more though, I wonder if it would hurt for counters to always be unsigned long. There is no problem on 32-bit as long and int are the same size. So on 64-bit, the cmpxchg_double path stays as it is. There would then be the extra miniscule risk that __cmpxchg_double_slab() fails due to a spurious _refcount modification due to an RCU-protected pagecache lookup. And there are a few places that would be a 64-bit load rather than a 32-bit load. I think if I were doing slub, I'd put in 'unsigned int counters_32' and 'unsigned long counters_64'. set_page_slub_counters() would then become simply: page->counters_32 = counters_new; -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
