Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1461567 > unrolled thread

Re: [PATCH v2] locking/percpu-rwsem: Optimize readers and reduce global impact

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] locking/percpu-rwsem: Optimize readers and reduce  global impact Om Dhyade <odhyade@codeaurora.org> - 2016-08-13 03:50 +0200
    Re: [PATCH v2] locking/percpu-rwsem: Optimize readers and reduce  global impact John Stultz <john.stultz@linaro.org> - 2016-08-24 23:20 +0200
      Re: [PATCH v2] locking/percpu-rwsem: Optimize readers and reduce  global impact Tejun Heo <tj@kernel.org> - 2016-08-24 23:40 +0200
        Re: [PATCH v2] locking/percpu-rwsem: Optimize readers and reduce  global impact John Stultz <john.stultz@linaro.org> - 2016-08-25 01:00 +0200

#1461567 — Re: [PATCH v2] locking/percpu-rwsem: Optimize readers and reduce global impact

Thank you Dimtry for sharing the patches.

Update from my tests:
Use-case: Android application launches.

I tested the patches on android N build, i see max latency ~7ms.
In my tests, the wait is due to: copy_process(fork.c) blocks all threads 
in __cgroup_procs_write including threads which are not part of the 
forking process's thread-group.

Dimtry had provided a hack patch which reverts to per-process rw-sem 
which had max latency of ~2ms.

android user-space binder library does 2 cgroup write operations per 
transaction, apart from the copy_process(fork.c) wait, i see pre-emption 
in _cgroup_procs_write causing waits.

Thanks.

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1469732

On Fri, Aug 12, 2016 at 6:44 PM, Om Dhyade <odhyade@codeaurora.org> wrote:
> Update from my tests:
> Use-case: Android application launches.
>
> I tested the patches on android N build, i see max latency ~7ms.
> In my tests, the wait is due to: copy_process(fork.c) blocks all threads in
> __cgroup_procs_write including threads which are not part of the forking
> process's thread-group.
>
> Dimtry had provided a hack patch which reverts to per-process rw-sem which
> had max latency of ~2ms.
>
> android user-space binder library does 2 cgroup write operations per
> transaction, apart from the copy_process(fork.c) wait, i see pre-emption in
> _cgroup_procs_write causing waits.


Hey Peter, Tejun, Oleg,
  So while you're tweaks for the percpu-rwsem have greatly helped the
regression folks were seeing (many thanks, by the way), as noted
above, the performance regression with the global lock compared to
earlier kernels is still ~3x slower (though again, much better then
the 80x slower that was seen earlier).

So I was wondering if patches to go back to the per signal_struct
locking would still be considered? Or is the global lock approach the
only way forward?

At a higher level, I'm worried that Android's use of cgroups as a
priority enforcement mechanism is at odds with developers focusing on
it as a container enforcement mechanism, as in the latter its not
common for tasks to change between cgroups, but with the former
priority adjustments are quite common.

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1469747

Hello, John.

On Wed, Aug 24, 2016 at 02:16:52PM -0700, John Stultz wrote:
> Hey Peter, Tejun, Oleg,
>   So while you're tweaks for the percpu-rwsem have greatly helped the
> regression folks were seeing (many thanks, by the way), as noted
> above, the performance regression with the global lock compared to
> earlier kernels is still ~3x slower (though again, much better then
> the 80x slower that was seen earlier).
> 
> So I was wondering if patches to go back to the per signal_struct
> locking would still be considered? Or is the global lock approach the
> only way forward?

We can't simply revert but we can make the lock per signal_struct
again.  It's just that it'd be quite a bit more complex (but, again,
if we need it...) and for cases where migrations aren't as frequent
percpu-rwsem would be at least a bit lower overhead.  Can you please
test with the following patch applied just in case?

 https://git.kernel.org/cgit/linux/kernel/git/tj/cgroup.git/commit/?h=for-4.8-fixes&id=568ac888215c7fb2fabe8ea739b00ec3c1f5d440

> At a higher level, I'm worried that Android's use of cgroups as a
> priority enforcement mechanism is at odds with developers focusing on
> it as a container enforcement mechanism, as in the latter its not
> common for tasks to change between cgroups, but with the former
> priority adjustments are quite common.

It has been at odds as long as android existed.  cgroup used to have
synchronous synchronize_rcu() in the migration path which android
kernel simply deleted (didn't break android's use case), re-labeling
every page's memcg ownership on foreground and background switches
(does it still do that? it's simply unworkable) and so on.

I find it difficult to believe that android's requirements can be
satisfied only through moving processes around.  cgroup usage model is
affected by container use cases but isn't limited to it.  If android
is willing to change, I'd be more than happy to work with you and
solve obstacles.  If not, we'll surely try not to break it anymore
than it has always been broken.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1469771

On Wed, Aug 24, 2016 at 2:30 PM, Tejun Heo <tj@kernel.org> wrote:
> On Wed, Aug 24, 2016 at 02:16:52PM -0700, John Stultz wrote:
>>
>> So I was wondering if patches to go back to the per signal_struct
>> locking would still be considered? Or is the global lock approach the
>> only way forward?
>
> We can't simply revert but we can make the lock per signal_struct
> again.  It's just that it'd be quite a bit more complex (but, again,
> if we need it...) and for cases where migrations aren't as frequent
> percpu-rwsem would be at least a bit lower overhead.  Can you please
> test with the following patch applied just in case?
>
>  https://git.kernel.org/cgit/linux/kernel/git/tj/cgroup.git/commit/?h=for-4.8-fixes&id=568ac888215c7fb2fabe8ea739b00ec3c1f5d440

That does provide a reasonable improvement in my testing!  But, I'll
pass it along to folks who are doing more in-depth performance
analysis to make sure.

If that doesn't work, I'll talk w/ Dmitry about submitting his patch
reworking things back to the per signal_struct locking.

>> At a higher level, I'm worried that Android's use of cgroups as a
>> priority enforcement mechanism is at odds with developers focusing on
>> it as a container enforcement mechanism, as in the latter its not
>> common for tasks to change between cgroups, but with the former
>> priority adjustments are quite common.
>
> It has been at odds as long as android existed.  cgroup used to have
> synchronous synchronize_rcu() in the migration path which android
> kernel simply deleted (didn't break android's use case), re-labeling
> every page's memcg ownership on foreground and background switches
> (does it still do that? it's simply unworkable) and so on.

Yea. Android folks can correct me, but I think the memcg
forground/background bits have been dropped. They still use a apps
memcg group for low-memory-notification in their
userland-low-memory-killer-daemon (however my understanding is that
has yet to sufficiently replace the in-kernel low-memory-killer).

> I find it difficult to believe that android's requirements can be
> satisfied only through moving processes around.  cgroup usage model is
> affected by container use cases but isn't limited to it.  If android
> is willing to change, I'd be more than happy to work with you and
> solve obstacles.  If not, we'll surely try not to break it anymore
> than it has always been broken.

I think folks are open to ideas, but going from idea-from-the-list to
something reliable enough to ship is always a bit fraught, so it often
takes some time and effort to really validate if those ideas are
workable.

I have on my list to re-submit the can_attach logic Android uses to
provide permissions checks on processes migrating other tasks between
cgroups, so at least we can try to reduce the separate domains of
focus and get folks sharing the same code bases.

thanks
-john

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web