diff mbox

arm: align shared memory unconditionally to the SHMLBA boundary

Message ID 1361254269-3444-1-git-send-email-alekskartashov@parallels.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alexander Kartashov Feb. 19, 2013, 6:11 a.m. UTC
Currently IPC SHM works in a strange way on ARM:
the syscall sys_shmat() requires the argument shmaddr
be SHMLBA-aligned (ARM has the macro __ARCH_FORCE_SHMLBA
unconditionally defined) but allocates memory that
isn't SHMLBA-aligned because the value of memory alignment
depends on presense of certain cache aliases.

Such a behavior raises the following problem. Consider
the program:

static int p[2];
static pid_t task2_pid;

static int task1()
{
	int task2_status;
	int *n;
	int shmid;

	shmid = shmget(0x94646337, 40960, IPC_CREAT);
	if (shmid < 0) {
		perror("Failed to get a SHM descriptor");
		return 10;
	}

	n = (int*)shmat(shmid, 0, 0);
	if (n == (int*)-1) {
		perror("task1: failed to attach the SHM segment");
		return 11;
	}

	*n = MAGIC;

	shmdt(n);

	if (write(p[1], &n, sizeof(n)) != sizeof(n))
	{
		perror("task1: failed to send the SHM address");
		return 12;
	}

	if (waitpid(task2_pid, &task2_status, 0) != task2_pid)
	{
		perror("task2: failed to wait for task2");
		return 13;
	}

	if (!WIFEXITED(task2_status))
	{
		printf("task1: task2 hasn't exited!\n");
		return 14;
	}

	printf("task1: task2 exited with code %d, "
		"the SHM was attached to %p.\n",
		WEXITSTATUS(task2_status), n);

	return 0;
}

static int task2()
{
	int *n = 0;
	int res;
	void *addr;
	int shmid;

	shmid = shmget(0x94646337, 40960, IPC_CREAT);
	if (shmid < 0) {
		perror("Failed to get a SHM descriptor");
		return 20;
	}

	if (read(p[0], &n, sizeof(n)) != sizeof(n))
	{
		perror("task2: failed to receive the SHM address");
		return 21;
	}

	addr = shmat(shmid, n, 0);
	if (addr == (void*)-1)
	{
		printf("task2: failed to attach the SHM to the address %p: %s\n",
			n, strerror(errno));
		res = 22;
		goto exit_task2;
	}

	if ((void*)n != addr)
	{
		printf("task2: the SHM isn't attached where it's expected:"
				"expected %p, got %p\n", n, addr);
		res = 23;
		goto exit_task2;
	}

	if (*n != MAGIC)
	{
		printf("task2: *n (%d) isn't %d\n", *n, MAGIC);
		res = 24;
		goto exit_task2;
	}

exit_task2:
	shmdt(addr);

	return res;
}

int main()
{
	int res = 0;

	if (pipe(p) < 0)
	{
		perror("Failed to create a pipe");
		return 2;
	}

	task2_pid = fork();
	if (task2_pid)
		res = task1();
	else
		res = task2();

	close(p[0]);
	close(p[1]);

	return res;
}

This program sometimes fails generating the output like that:

root@test:~/shmfail# ./shmfail
task2: failed to attach the SHM to the address 0xb6e3a000: Invalid argument
task1: task2 exited with code 22, the SHM was attached to 0xb6e3a000.

root@test:~/shmfail# ./shmfail
task2: failed to attach the SHM to the address 0xb6e63000: Invalid argument
task1: task2 exited with code 22, the SHM was attached to 0xb6e63000.

and sometimes succeeds:

root@test:~/shmfail# ./shmfail
task1: task2 exited with code 0, the SHM was attached to 0xb6e94000.

The problem is reproducible in the mainstream kernel branch as well as
in a stable (3.7.5) kernel release. The program isn't reproducible on x86_64.

As you can see the program fails when the address returned by
the function shmat() isn't SHMLBA-aligned.

This patch makes file-mmapped memory be unconditionaly
SHMLBA-aligned.

Signed-off-by: Alexander Kartashov <alekskartashov@parallels.com>
CC: Russell King - ARM Linux <linux@arm.linux.org.uk>
CC: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
---
 arch/arm/mm/mmap.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Cyrill Gorcunov July 15, 2013, 5:32 p.m. UTC | #1
On Tue, Feb 19, 2013 at 10:11:09AM +0400, Alexander Kartashov wrote:
> Currently IPC SHM works in a strange way on ARM:
> the syscall sys_shmat() requires the argument shmaddr
> be SHMLBA-aligned (ARM has the macro __ARCH_FORCE_SHMLBA
> unconditionally defined) but allocates memory that
> isn't SHMLBA-aligned because the value of memory alignment
> depends on presense of certain cache aliases.

Hi guys, is there some conclusion on this patch? It has been sent
almost 5 months back ;)
Russell King - ARM Linux July 15, 2013, 6:08 p.m. UTC | #2
On Mon, Jul 15, 2013 at 09:32:38PM +0400, Cyrill Gorcunov wrote:
> On Tue, Feb 19, 2013 at 10:11:09AM +0400, Alexander Kartashov wrote:
> > Currently IPC SHM works in a strange way on ARM:
> > the syscall sys_shmat() requires the argument shmaddr
> > be SHMLBA-aligned (ARM has the macro __ARCH_FORCE_SHMLBA
> > unconditionally defined) but allocates memory that
> > isn't SHMLBA-aligned because the value of memory alignment
> > depends on presense of certain cache aliases.
> 
> Hi guys, is there some conclusion on this patch? It has been sent
> almost 5 months back ;)

It's pointless.  The alignment is only required for CPUs which have
aliasing caches.  What the code does in mmap() is correct.  However,
we can't dynamically select this in the SHM code.
Cyrill Gorcunov July 15, 2013, 6:57 p.m. UTC | #3
On Mon, Jul 15, 2013 at 07:08:46PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 15, 2013 at 09:32:38PM +0400, Cyrill Gorcunov wrote:
> > On Tue, Feb 19, 2013 at 10:11:09AM +0400, Alexander Kartashov wrote:
> > > Currently IPC SHM works in a strange way on ARM:
> > > the syscall sys_shmat() requires the argument shmaddr
> > > be SHMLBA-aligned (ARM has the macro __ARCH_FORCE_SHMLBA
> > > unconditionally defined) but allocates memory that
> > > isn't SHMLBA-aligned because the value of memory alignment
> > > depends on presense of certain cache aliases.
> > 
> > Hi guys, is there some conclusion on this patch? It has been sent
> > almost 5 months back ;)
> 
> It's pointless.  The alignment is only required for CPUs which have
> aliasing caches.  What the code does in mmap() is correct.  However,
> we can't dynamically select this in the SHM code.

Hi Russell, thanks a lot for reply! I must admit I'm not that familiar
with arm hw internals, but if the behaviour is correct than we've a
problem with checkpoint/restore procedure on arm platform :( As far
as I know Alexander pointed me that he find no other way of fixing
the problem except patching the kernel. Alexander, mind to remind
where exactly we're failing in c/r procedure, thus maybe arm guys
would help us to find a way to pass it by with some user-space
trick?
Alexander Kartashov July 16, 2013, 5:37 a.m. UTC | #4
On 07/15/2013 10:57 PM, Cyrill Gorcunov wrote:
> Alexander, mind to remind
> where exactly we're failing in c/r procedure

It isn't always possible to pin an IPC SHM region
using shmat() at the address returned by shmget()
so sometimes it's impossible to restore a process
that uses the IPC SHM.
Cyrill Gorcunov July 16, 2013, 9:53 a.m. UTC | #5
On Tue, Jul 16, 2013 at 09:37:15AM +0400, Alexander Kartashov wrote:
> On 07/15/2013 10:57 PM, Cyrill Gorcunov wrote:
> >Alexander, mind to remind where exactly we're failing in c/r procedure
> 
> It isn't always possible to pin an IPC SHM region
> using shmat() at the address returned by shmget()
> so sometimes it's impossible to restore a process
> that uses the IPC SHM.

Does it mean that we simply can't checkpoint and restore on same
node (ie same arm family) if this patch is not applied, right?
Is there any possibility to somehow workaround this problem
completely in user-space?
Alexander Kartashov July 16, 2013, 10:22 a.m. UTC | #6
On 07/16/2013 01:53 PM, Cyrill Gorcunov wrote:
> Does it mean that we simply can't checkpoint and restore on same
> node (ie same arm family) if this patch is not applied, right?
Yes, it does.
> Is there any possibility to somehow workaround this problem
> completely in user-space?
No, it's impossible since an IPC SHM region is allocated
by the routine shmget() in the dumpee; if the routine
returns an incorrectly aligned address it's impossible
to reattach the region at this address while restoring
the dumpee.
Russell King - ARM Linux July 16, 2013, 10:36 a.m. UTC | #7
On Tue, Jul 16, 2013 at 02:22:41PM +0400, Alexander Kartashov wrote:
> On 07/16/2013 01:53 PM, Cyrill Gorcunov wrote:
>> Does it mean that we simply can't checkpoint and restore on same
>> node (ie same arm family) if this patch is not applied, right?
> Yes, it does.
>> Is there any possibility to somehow workaround this problem
>> completely in user-space?
> No, it's impossible since an IPC SHM region is allocated
> by the routine shmget() in the dumpee; if the routine
> returns an incorrectly aligned address it's impossible
> to reattach the region at this address while restoring
> the dumpee.

shmget() doesn't allocate space in the process for the SHM region.  It
merely creates the shm memory and returns an identifier for it which can
later be used by shmat() to map it.
Alexander Kartashov July 16, 2013, 10:47 a.m. UTC | #8
On 07/16/2013 02:36 PM, Russell King - ARM Linux wrote:
> shmget() doesn't allocate space in the process for the SHM region.  It
> merely creates the shm memory and returns an identifier for it which can
> later be used by shmat() to map it.
Thank you for the correction, I meant shmat() in that comment indeed.
I'm sorry for the inconvenience.
diff mbox

Patch

diff --git a/arch/arm/mm/mmap.c b/arch/arm/mm/mmap.c
index 10062ce..eca577e3 100644
--- a/arch/arm/mm/mmap.c
+++ b/arch/arm/mm/mmap.c
@@ -65,8 +65,7 @@  arch_get_unmapped_area(struct file *filp, unsigned long addr,
 	 * We only need to do colour alignment if either the I or D
 	 * caches alias.
 	 */
-	if (aliasing)
-		do_align = filp || (flags & MAP_SHARED);
+	do_align = filp || (flags & MAP_SHARED);
 
 	/*
 	 * We enforce the MAP_FIXED case.