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

Groups > linux.kernel > #1641955 > unrolled thread

Re: [v3 0/9] parallelized "struct page" zeroing

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: [v3 0/9] parallelized "struct page" zeroing Pasha Tatashin <pasha.tatashin@oracle.com> - 2017-05-15 20:20 +0200
    Re: [v3 0/9] parallelized "struct page" zeroing Michal Hocko <mhocko@kernel.org> - 2017-05-15 21:40 +0200
      Re: [v3 0/9] parallelized "struct page" zeroing Pasha Tatashin <pasha.tatashin@oracle.com> - 2017-05-15 22:50 +0200
        Re: [v3 0/9] parallelized "struct page" zeroing Michal Hocko <mhocko@kernel.org> - 2017-05-16 10:40 +0200

#1641955 — Re: [v3 0/9] parallelized "struct page" zeroing

Hi Michal,

After looking at your suggested memblock_virt_alloc_core() change again, 
I decided to keep what I have. I do not want to inline 
memblock_virt_alloc_internal(), because it is not a performance critical 
path, and by inlining it we will unnecessarily increase the text size on 
all platforms.

Also, because it will be very hard to make sure that no platform 
regresses by making memset() default in _memblock_virt_alloc_core() (as 
I already showed last week at least sun4v SPARC64 will require special 
changes in order for this to work), I decided to make it available only 
for "deferred struct page init" case. As, what is already in the patch.

I am working on testing to make sure we do not need to double zero in 
the two cases that you found: sparsemem, and mem hotplug. Please let me 
know if you have any more comments, or if I can send new patches out 
when they are ready.

Thank you,
Pasha

[toc] | [next] | [standalone]

#1642003

On Mon 15-05-17 14:12:10, Pasha Tatashin wrote:
> Hi Michal,
> 
> After looking at your suggested memblock_virt_alloc_core() change again, I
> decided to keep what I have. I do not want to inline
> memblock_virt_alloc_internal(), because it is not a performance critical
> path, and by inlining it we will unnecessarily increase the text size on all
> platforms.

I do not insist but I would really _prefer_ if the bool zero argument
didn't proliferate all over the memblock API.
 
> Also, because it will be very hard to make sure that no platform regresses
> by making memset() default in _memblock_virt_alloc_core() (as I already
> showed last week at least sun4v SPARC64 will require special changes in
> order for this to work), I decided to make it available only for "deferred
> struct page init" case. As, what is already in the patch.

I do not think this is the right approach. Your measurements just show
that sparc could have a more optimized memset for small sizes. If you
keep the same memset only for the parallel initialization then you
just hide this fact. I wouldn't worry about other architectures. All
sane architectures should simply work reasonably well when touching a
single or only few cache lines at the same time. If some arches really
suffer from small memsets then the initialization should be driven by a
specific ARCH_WANT_LARGE_PAGEBLOCK_INIT rather than making this depend
on DEFERRED_INIT. Or if you are too worried then make it opt-in and make
it depend on ARCH_WANT_PER_PAGE_INIT and make it enabled for x86 and
sparc after memset optimization.

-- 
Michal Hocko
SUSE Labs

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

#1642048

On 05/15/2017 03:38 PM, Michal Hocko wrote:
> On Mon 15-05-17 14:12:10, Pasha Tatashin wrote:
>> Hi Michal,
>>
>> After looking at your suggested memblock_virt_alloc_core() change again, I
>> decided to keep what I have. I do not want to inline
>> memblock_virt_alloc_internal(), because it is not a performance critical
>> path, and by inlining it we will unnecessarily increase the text size on all
>> platforms.
> 
> I do not insist but I would really _prefer_ if the bool zero argument
> didn't proliferate all over the memblock API.

Sure, I will remove zero boolean argument from 
memblock_virt_alloc_internal(), and do memset() calls inside callers.

>   
>> Also, because it will be very hard to make sure that no platform regresses
>> by making memset() default in _memblock_virt_alloc_core() (as I already
>> showed last week at least sun4v SPARC64 will require special changes in
>> order for this to work), I decided to make it available only for "deferred
>> struct page init" case. As, what is already in the patch.
> 
> I do not think this is the right approach. Your measurements just show
> that sparc could have a more optimized memset for small sizes. If you
> keep the same memset only for the parallel initialization then you
> just hide this fact. I wouldn't worry about other architectures. All
> sane architectures should simply work reasonably well when touching a
> single or only few cache lines at the same time. If some arches really
> suffer from small memsets then the initialization should be driven by a
> specific ARCH_WANT_LARGE_PAGEBLOCK_INIT rather than making this depend
> on DEFERRED_INIT. Or if you are too worried then make it opt-in and make
> it depend on ARCH_WANT_PER_PAGE_INIT and make it enabled for x86 and
> sparc after memset optimization.

OK, I will think about this.

I do not really like adding new configs because they tend to clutter the 
code. This is why, I wanted to rely on already existing config that I 
know benefits all platforms that use it. Eventually, 
"CONFIG_DEFERRED_STRUCT_PAGE_INIT" is going to become the default 
everywhere, as there should not be a drawback of using it even on small 
machines.

Pasha

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

#1642310

On Mon 15-05-17 16:44:26, Pasha Tatashin wrote:
> On 05/15/2017 03:38 PM, Michal Hocko wrote:
> >I do not think this is the right approach. Your measurements just show
> >that sparc could have a more optimized memset for small sizes. If you
> >keep the same memset only for the parallel initialization then you
> >just hide this fact. I wouldn't worry about other architectures. All
> >sane architectures should simply work reasonably well when touching a
> >single or only few cache lines at the same time. If some arches really
> >suffer from small memsets then the initialization should be driven by a
> >specific ARCH_WANT_LARGE_PAGEBLOCK_INIT rather than making this depend
> >on DEFERRED_INIT. Or if you are too worried then make it opt-in and make
> >it depend on ARCH_WANT_PER_PAGE_INIT and make it enabled for x86 and
> >sparc after memset optimization.
> 
> OK, I will think about this.
> 
> I do not really like adding new configs because they tend to clutter the
> code. This is why,

Yes I hate adding new (arch) config options as well. And I still believe
we do not need any here either...

> I wanted to rely on already existing config that I know benefits all
> platforms that use it.

I wouldn't be so sure about this. If any other platform has a similar
issues with small memset as sparc then the overhead is just papered over
by parallel initialization.

> Eventually,
> "CONFIG_DEFERRED_STRUCT_PAGE_INIT" is going to become the default
> everywhere, as there should not be a drawback of using it even on small
> machines.

Maybe and I would highly appreciate that.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web