[CRIU] [PATCH] rst alloc: align on reporting cpos too

[Andrew Vagin]

On Wed, Feb 10, 2016 at 09:47:46AM -0700, Tycho Andersen wrote:
> On Wed, Feb 10, 2016 at 08:43:37AM -0800, Andrew Vagin wrote:
> > On Wed, Feb 10, 2016 at 06:02:37PM +0300, Pavel Emelyanov wrote:
> > > On 02/10/2016 05:44 PM, Tycho Andersen wrote:
> > > > On Wed, Feb 10, 2016 at 04:37:59PM +0300, Pavel Emelyanov wrote:
> > > >> On 02/09/2016 08:38 PM, Tycho Andersen wrote:
> > > >>> Since we align in rst_mem_alloc, we should also align when reporting the
> > > >>> current position; if we don't and things get unlucky, we report a different
> > > >>> position than where the pointer is actually allocated, which fucks things
> > > >>> up quite bad :)
> > > >>
> > > >> Applied, thanks.
> > > >>
> > > >> However, if I do this:
> > > >>
> > > >> x = rst_mem_cpos()
> > > >> x2 = rst_mem_alloc_cont()
> > > >>
> > > >> then x will not correspond to x2 since the latter doesn't align what
> > > >> it allocates.
> > > > 
> > > > Oh, yep :(.
> > > 
> > > But I'm not sure whether anyone uses this. Let me check...
> > > 
> > > I'm afraid, but it looks like someone does:
> > > 
> > > * prepare_posix_timers
> > > * prepare_rlimits
> > > * prepare_signals
> > > * ...
> > > 
> > > :(
> > > 
> > > >> I've proposed to introduce a call rst_mem_alogn() that would just
> > > >> shift the "free" pointer up to the nearest aligned value. With this
> > > >> any combination would work.
> > > > 
> > > > Not sure I understand the call pattern you're proposing here. Can you
> > > > elaborate? IIUC, the only way to fix this is to add a
> > > > rst_mem_cpos_cont() call that is used with rst_mem_alloc_cont(), and
> > > > then go fix each call site. Maybe that's what you were suggesting and
> > > > I misunderstood :)
> > > 
> > > Yes, this could be an option, but thus we'd have 4 calls -- rst_mem_alloc,
> > > _cpos and 2 more with _cont suffix. My proposal is to revert all of this
> > > including Andrey's patch and just introduce a _single_ call
> > > 
> > > rst_mem_align()
> > > 
> > > which would take struct rst_mem_type *t = &rst_mems[type] and the would
> > > adjust t->free_mem = align(t->free_mem, sizeof(void *)). And that's it.
> > > After this call _alloc() and _cpos() would report "good" and coniciding
> > > values.
> > > 
> > > For places that need aligned pointers (that currently just call _alloc())
> > > we'll have to precede them with calls to rst_mem_alloc()
> > 
> > Why do think that we don't need to align a start adress of an array?
> 
> I don't know much about why we need to align things in the first
> place, so it's entirely possible we do need to :)

Here are a few things why we need to allign data
* perfomance
https://software.intel.com/en-us/articles/coding-for-performance-data-alignment-and-structures

* Atomic operations works only for aligned data
For example:

        /*
         * The futex address must be "naturally" aligned.
         */
        key->both.offset = address % PAGE_SIZE;
        if (unlikely((address % sizeof(u32)) != 0))
                return -EINVAL;


Here is a note about malloc():

The address of a block returned by malloc or realloc in GNU systems is
always a multiple of eight (or sixteen on 64-bit systems). If you need a
block whose address is a multiple of a higher power of two than that,
use aligned_alloc or posix_memalign. aligned_alloc and posix_memalign
are declared in stdlib.h.

> 
> 7458b05 says "if we don't align a pointer, futex and atomic operations
> can fail." So I think we only need to align arrays whose members may
> be passed to futex or atomic. Is that right?
> 
> > I can call rst_mem_alloc(type, 1) and then start to allocate a new
> > array.
> > 
> > > 
> > > > Tycho
> > > > 
> > > >> -- Pavel
> > > >>
> > > >>> Closes #111
> > > >>>
> > > >>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > > >>> ---
> > > >>>  rst-malloc.c | 2 +-
> > > >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>>
> > > >>> diff --git a/rst-malloc.c b/rst-malloc.c
> > > >>> index 3556980..8e17255 100644
> > > >>> --- a/rst-malloc.c
> > > >>> +++ b/rst-malloc.c
> > > >>> @@ -128,7 +128,7 @@ unsigned long rst_mem_cpos(int type)
> > > >>>  {
> > > >>>  	struct rst_mem_type_s *t = &rst_mems[type];
> > > >>>  	BUG_ON(!t->remapable || !t->enabled);
> > > >>> -	return t->free_mem - t->buf;
> > > >>> +	return ((void*) round_up((unsigned long)t->free_mem, sizeof(void *))) - t->buf;
> > > >>>  }
> > > >>>  
> > > >>>  void *rst_mem_remap_ptr(unsigned long pos, int type)
> > > >>>
> > > >>
> > > > .
> > > > 
> > >