The user_shm_lock and user_shm_unlock functions use a single global
spinlock for protecting the user->locked_shm.
This is an overhead for multiple CPUs calling this code even if they
are having different user_struct.
Remove the global shmlock_user_lock and introduce and use a new
spinlock inside of the user_struct structure.
Signed-off-by: rajman mekaco <[email protected]>
---
include/linux/sched.h | 1 +
kernel/user.c | 1 +
mm/mlock.c | 10 ++++------
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 81a173c..c661cfd 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -720,6 +720,7 @@ struct user_struct {
unsigned long mq_bytes; /* How many bytes can be allocated to mqueue? */
#endif
unsigned long locked_shm; /* How many pages of mlocked shm ? */
+ spinlock_t shmlock_user_lock; /* Protects locked_shm */
#ifdef CONFIG_KEYS
struct key *uid_keyring; /* UID specific keyring */
diff --git a/kernel/user.c b/kernel/user.c
index 71dd236..ca7f423 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -169,6 +169,7 @@ struct user_struct *alloc_uid(struct user_namespace *ns, uid_t uid)
} else {
uid_hash_insert(new, hashent);
up = new;
+ spin_lock_init(&new->shmlock_user_lock);
}
spin_unlock_irq(&uidhash_lock);
}
diff --git a/mm/mlock.c b/mm/mlock.c
index ef726e8..11a78a6 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -593,8 +593,6 @@ SYSCALL_DEFINE0(munlockall)
* Objects with different lifetime than processes (SHM_LOCK and SHM_HUGETLB
* shm segments) get accounted against the user_struct instead.
*/
-static DEFINE_SPINLOCK(shmlock_user_lock);
-
int user_shm_lock(size_t size, struct user_struct *user)
{
unsigned long lock_limit, locked;
@@ -605,7 +603,7 @@ int user_shm_lock(size_t size, struct user_struct *user)
if (lock_limit == RLIM_INFINITY)
allowed = 1;
lock_limit >>= PAGE_SHIFT;
- spin_lock(&shmlock_user_lock);
+ spin_lock(&user->shmlock_user_lock);
if (!allowed &&
locked + user->locked_shm > lock_limit && !capable(CAP_IPC_LOCK))
goto out;
@@ -613,14 +611,14 @@ int user_shm_lock(size_t size, struct user_struct *user)
user->locked_shm += locked;
allowed = 1;
out:
- spin_unlock(&shmlock_user_lock);
+ spin_unlock(&user->shmlock_user_lock);
return allowed;
}
void user_shm_unlock(size_t size, struct user_struct *user)
{
- spin_lock(&shmlock_user_lock);
+ spin_lock(&user->shmlock_user_lock);
user->locked_shm -= (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
- spin_unlock(&shmlock_user_lock);
+ spin_unlock(&user->shmlock_user_lock);
free_uid(user);
}
--
1.7.5.4
On 05/03/2012 01:34 PM, rajman mekaco wrote:
> The user_shm_lock and user_shm_unlock functions use a single global
> spinlock for protecting the user->locked_shm.
>
> This is an overhead for multiple CPUs calling this code even if they
> are having different user_struct.
>
> Remove the global shmlock_user_lock and introduce and use a new
> spinlock inside of the user_struct structure.
>
> Signed-off-by: rajman mekaco<[email protected]>
Reviewed-by: Rik van Riel <[email protected]>
On Thu, 2012-05-03 at 23:04 +0530, rajman mekaco wrote:
> The user_shm_lock and user_shm_unlock functions use a single global
> spinlock for protecting the user->locked_shm.
Are you very sure its only protecting user state? This changelog doesn't
convince me you've gone through everything and found it good.
> This is an overhead for multiple CPUs calling this code even if they
> are having different user_struct.
>
> Remove the global shmlock_user_lock and introduce and use a new
> spinlock inside of the user_struct structure.
While I don't immediately see anything wrong with it, I doubt its
useful. What workload run with enough users that this makes a difference
one way or another?
On 05/03/2012 03:31 PM, Peter Zijlstra wrote:
> On Thu, 2012-05-03 at 23:04 +0530, rajman mekaco wrote:
>> The user_shm_lock and user_shm_unlock functions use a single global
>> spinlock for protecting the user->locked_shm.
>
> Are you very sure its only protecting user state? This changelog doesn't
> convince me you've gone through everything and found it good.
>
>> This is an overhead for multiple CPUs calling this code even if they
>> are having different user_struct.
>>
>> Remove the global shmlock_user_lock and introduce and use a new
>> spinlock inside of the user_struct structure.
>
> While I don't immediately see anything wrong with it, I doubt its
> useful. What workload run with enough users that this makes a difference
> one way or another?
When running with containers and/or LXC, I believe that
each UID in each container gets its own user_struct, but
you do raise a good question - what user programs call
mlock anyway, and how often?
On 05/03/2012 02:07 PM, Rik van Riel wrote:
> On 05/03/2012 01:34 PM, rajman mekaco wrote:
>> The user_shm_lock and user_shm_unlock functions use a single global
>> spinlock for protecting the user->locked_shm.
>>
>> This is an overhead for multiple CPUs calling this code even if they
>> are having different user_struct.
>>
>> Remove the global shmlock_user_lock and introduce and use a new
>> spinlock inside of the user_struct structure.
>>
>> Signed-off-by: rajman mekaco<[email protected]>
>
> Reviewed-by: Rik van Riel <[email protected]>
Hold this ... while the patch is correct, Peter raised
a valid concern about its usefulness, which should be
sorted out first.
Thank you all for replying back.
>
> Hold this ... while the patch is correct, Peter raised
> a valid concern about its usefulness, which should be
> sorted out first.
>
Can't the shmctl(SHM_LOCK) system call be called for a huge number of
usermode processes ?
Other place from where usr_shm_lock() is called is for hugetlb from
shmget(SHM_HUGETLB)
system call via ipc_get().
As far as users are concerned, I think that if even 2 user_structs
encounter this on 2 different CPUs,
why should the processors waste any time at all at looping even once
if they belong to different
user_structs ?
I totally agree with you that maybe if we look at the entire workloads
it probably wouldn't matter much
because of low number of users, but why should the CPUs compete and
spin for different users at all
when nothing global is affected ?
Hi,
Any updates on this ?
On 05/10/2012 09:34 AM, rajman mekaco wrote:
> Any updates on this ?
There is still no usecase to demonstrate a problem, so no real
justification to merge the patch. Coming up with such a usecase
is up to the submitter of the patch.
--
All rights reversed
On Thu, May 10, 2012 at 8:24 PM, Rik van Riel <[email protected]> wrote:
> On 05/10/2012 09:34 AM, rajman mekaco wrote:
>
>> Any updates on this ?
>
>
> There is still no usecase to demonstrate a problem, so no real
> justification to merge the patch. ?Coming up with such a usecase
> is up to the submitter of the patch.
Maybe you didn't read my last email:
If 2 different user-mode processes executing on 2 CPUs under 2 different
users want to access the same shared memory through the
shmctl(SHM_LOCK) / shmget(SHM_HUGETLB) / usr_shm_lock
primitives, they could compete/spin even though their user_structs
are different.
Can you please correct me if I am missing some crucial point of understanding ?
Or did you mean that I should update the ChangeLog with this kind of
description ?
>
> --
> All rights reversed
> If 2 different user-mode processes executing on 2 CPUs under 2 different
> users want to access the same shared memory through the
One correction:
This will happen even for different shared memory as the lock is global.
This fact just increases the relevance of this patch, dont you think ?
> shmctl(SHM_LOCK) / shmget(SHM_HUGETLB) / usr_shm_lock
> primitives, they could compete/spin even though their user_structs
> are different.
>
On 05/10/2012 11:39 AM, rajman mekaco wrote:
> On Thu, May 10, 2012 at 8:24 PM, Rik van Riel<[email protected]> wrote:
>> On 05/10/2012 09:34 AM, rajman mekaco wrote:
>>
>>> Any updates on this ?
>>
>>
>> There is still no usecase to demonstrate a problem, so no real
>> justification to merge the patch. Coming up with such a usecase
>> is up to the submitter of the patch.
>
> Maybe you didn't read my last email:
> If 2 different user-mode processes executing on 2 CPUs under 2 different
> users want to access the same shared memory through the
> shmctl(SHM_LOCK) / shmget(SHM_HUGETLB) / usr_shm_lock
> primitives, they could compete/spin even though their user_structs
> are different.
>
> Can you please correct me if I am missing some crucial point of understanding ?
Mlock is a very very expensive operation.
Updating the mlock statistics is a very cheap operation.
Does this spinlock ever show up contention wise?
--
All rights reversed
>>
>> Maybe you didn't read my last email:
>> If 2 different user-mode processes executing on 2 CPUs under 2 different
>> users want to access the same shared memory through the
>> shmctl(SHM_LOCK) / shmget(SHM_HUGETLB) / usr_shm_lock
>> primitives, they could compete/spin even though their user_structs
>> are different.
>>
>> Can you please correct me if I am missing some crucial point of
>> understanding ?
>
>
> Mlock is a very very expensive operation.
>
> Updating the mlock statistics is a very cheap operation.
>
> Does this spinlock ever show up contention wise?
I just tested for working and not contention. :)
I was just going by correctness of concept.
But I understand what you say and I will try to actually test contention
for this in the coming days.
>
> --
> All rights reversed