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

Groups > linux.kernel > #1547515 > unrolled thread

Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations

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 v4 0/9] mm/swap: Regular page swap optimizations Minchan Kim <minchan@kernel.org> - 2016-12-27 08:50 +0100
    Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations "Huang\, Ying" <ying.huang@intel.com> - 2016-12-28 03:00 +0100
      Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations Minchan Kim <minchan@kernel.org> - 2016-12-28 03:40 +0100
        Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations "Huang\, Ying" <ying.huang@intel.com> - 2016-12-28 04:20 +0100
          Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations "Huang\, Ying" <ying.huang@intel.com> - 2016-12-28 04:40 +0100
            Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations Minchan Kim <minchan@kernel.org> - 2016-12-28 05:10 +0100
              Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations "Huang\, Ying" <ying.huang@intel.com> - 2016-12-28 06:00 +0100
    Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations Jan Kara <jack@suse.cz> - 2017-01-02 16:50 +0100
      Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations Minchan Kim <minchan@kernel.org> - 2017-01-03 05:40 +0100
        Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations "Huang\, Ying" <ying.huang@intel.com> - 2017-01-03 06:50 +0100

#1547515 — Re: [PATCH v4 0/9] mm/swap: Regular page swap optimizations

Hi,

On Fri, Dec 09, 2016 at 01:09:13PM -0800, Tim Chen wrote:
> Change Log:
> v4:
> 1. Fix a bug in unlock cluster in add_swap_count_continuation(). We
> should use unlock_cluster() instead of unlock_cluser_or_swap_info().
> 2. During swap off, handle race when swap slot is marked unused but allocated,
> and not yet placed in swap cache.  Wait for swap slot to be placed in swap cache
> and not abort swap off.
> 3. Initialize n_ret in get_swap_pages().
> 
> v3:
> 1. Fix bug that didn't check for page already in swap cache before skipping
> read ahead and return null page.
> 2. Fix bug that didn't try to allocate from global pool if allocation
> from swap slot cache did not succeed.
> 3. Fix memory allocation bug for spaces to store split up 64MB radix tree
> 4. Fix problems caused by races between get_swap_page, cpu online/offline and
> swap_on/off
> 
> v2: 
> 1. Fix bug in the index limit used in scan_swap_map_try_ssd_cluster
> when searching for empty slots in cluster.
> 2. Fix bug in swap off that incorrectly determines if we still have
> swap devices left.
> 3. Port patches to mmotm-2016-10-11-15-46 branch
> 
> Andrew,
> 
> We're updating this patch series with some minor fixes.
> Please consider this patch series for inclusion to 4.10.
>  
> Times have changed.  Coming generation of Solid state Block device
> latencies are getting down to sub 100 usec, which is within an order of
> magnitude of DRAM, and their performance is orders of magnitude higher
> than the single- spindle rotational media we've swapped to historically.
> 
> This could benefit many usage scenearios.  For example cloud providers who
> overcommit their memory (as VM don't use all the memory provisioned).
> Having a fast swap will allow them to be more aggressive in memory
> overcommit and fit more VMs to a platform.
> 
> In our testing [see footnote], the median latency that the
> kernel adds to a page fault is 15 usec, which comes quite close
> to the amount that will be contributed by the underlying I/O
> devices.
> 
> The software latency comes mostly from contentions on the locks
> protecting the radix tree of the swap cache and also the locks protecting
> the individual swap devices.  The lock contentions already consumed
> 35% of cpu cycles in our test.  In the very near future,
> software latency will become the bottleneck to swap performnace as
> block device I/O latency gets within the shouting distance of DRAM speed.
> 
> This patch set, plus a previous patch Ying already posted
> (commit: f6498b3f) reduced the median page fault latency
> from 15 usec to 4 usec (375% reduction) for DRAM based pmem
> block device.

The patchset has used several techniqueus to reduce lock contention, for example,
batching alloc/free, fine-grained lock and cluster distribution to avoid cache
false-sharing. Each items has different complexity and benefits so could you
show the number for each step of pathchset? It would be better to include the
nubmer in each description. It helps how the patch is important when we consider
complexitiy of the patch.

> 
> Patch 1 is a clean up patch.

Could it be separated patch?

> Patch 2 creates a lock per cluster, this gives us a more fine graind lock
>         that can be used for accessing swap_map, and not lock the whole
>         swap device

I hope you make three steps to review easier. You can create some functions like
swap_map_lock and cluster_lock which are wrapper functions just hold swap_lock.
It doesn't change anything performance pov but it clearly shows what kinds of lock
we should use in specific context.

Then, you can introduce more fine-graind lock in next patch and apply it into
those wrapper functions.

And last patch, you can adjust cluster distribution to avoid false-sharing.
And the description should include how it's bad in testing so it's worth.

Frankly speaking, although I'm huge user of bit_spin_lock(zram/zsmalloc
have used it heavily), I don't like swap subsystem uses it.
During zram development, it really hurts debugging due to losing lockdep.
The reason zram have used it is by size concern of embedded world but server
would be not critical so please consider trade-off of spinlock vs. bit_spin_lock.

> Patch 3 splits the swap cache radix tree into 64MB chunks, reducing
>         the rate that we have to contende for the radix tree.

To me, it's rather hacky. I think it might be common problem for page cache
so can we think another generalized way like range_lock? Ccing Jan.

> Patch 4 eliminates unnecessary page allocation for read ahead.

Could it be separated patch?

> Patch 5-9 create a per cpu cache of the swap slots, so we don't have
>         to contend on the swap device to get a swap slot or to release
>         a swap slot.  And we allocate and release the swap slots
>         in batches for better efficiency.


To me, idea is good although I feel the amount of code is rather huge and
messy so it should include the number about the benefit, at least.

And it might make some of patches in this patchset if we put this batching
ahead before other patches redundant.

Sorry for vague commenting. In this phase, it's really hard to review.

[toc] | [next] | [standalone]

#1547809

Hi, Minchan,

Minchan Kim <minchan@kernel.org> writes:

> Hi,
>
> On Fri, Dec 09, 2016 at 01:09:13PM -0800, Tim Chen wrote:
>> Change Log:
>> v4:
>> 1. Fix a bug in unlock cluster in add_swap_count_continuation(). We
>> should use unlock_cluster() instead of unlock_cluser_or_swap_info().
>> 2. During swap off, handle race when swap slot is marked unused but allocated,
>> and not yet placed in swap cache.  Wait for swap slot to be placed in swap cache
>> and not abort swap off.
>> 3. Initialize n_ret in get_swap_pages().
>> 
>> v3:
>> 1. Fix bug that didn't check for page already in swap cache before skipping
>> read ahead and return null page.
>> 2. Fix bug that didn't try to allocate from global pool if allocation
>> from swap slot cache did not succeed.
>> 3. Fix memory allocation bug for spaces to store split up 64MB radix tree
>> 4. Fix problems caused by races between get_swap_page, cpu online/offline and
>> swap_on/off
>> 
>> v2: 
>> 1. Fix bug in the index limit used in scan_swap_map_try_ssd_cluster
>> when searching for empty slots in cluster.
>> 2. Fix bug in swap off that incorrectly determines if we still have
>> swap devices left.
>> 3. Port patches to mmotm-2016-10-11-15-46 branch
>> 
>> Andrew,
>> 
>> We're updating this patch series with some minor fixes.
>> Please consider this patch series for inclusion to 4.10.
>>  
>> Times have changed.  Coming generation of Solid state Block device
>> latencies are getting down to sub 100 usec, which is within an order of
>> magnitude of DRAM, and their performance is orders of magnitude higher
>> than the single- spindle rotational media we've swapped to historically.
>> 
>> This could benefit many usage scenearios.  For example cloud providers who
>> overcommit their memory (as VM don't use all the memory provisioned).
>> Having a fast swap will allow them to be more aggressive in memory
>> overcommit and fit more VMs to a platform.
>> 
>> In our testing [see footnote], the median latency that the
>> kernel adds to a page fault is 15 usec, which comes quite close
>> to the amount that will be contributed by the underlying I/O
>> devices.
>> 
>> The software latency comes mostly from contentions on the locks
>> protecting the radix tree of the swap cache and also the locks protecting
>> the individual swap devices.  The lock contentions already consumed
>> 35% of cpu cycles in our test.  In the very near future,
>> software latency will become the bottleneck to swap performnace as
>> block device I/O latency gets within the shouting distance of DRAM speed.
>> 
>> This patch set, plus a previous patch Ying already posted
>> (commit: f6498b3f) reduced the median page fault latency
>> from 15 usec to 4 usec (375% reduction) for DRAM based pmem
>> block device.
>
> The patchset has used several techniqueus to reduce lock contention, for example,
> batching alloc/free, fine-grained lock and cluster distribution to avoid cache
> false-sharing. Each items has different complexity and benefits so could you
> show the number for each step of pathchset? It would be better to include the
> nubmer in each description. It helps how the patch is important when we consider
> complexitiy of the patch.

One common problem of scalability optimization is that, after you have
optimized one lock, the end result may be not very good, because another
lock becomes heavily contended.  Similar problem occurs here, there are
mainly two locks during swap out/in, one protects swap cache, the other
protects swap device.  We can achieve good scalability only after having
optimized the two locks.

You cannot say that one patch is not important just because the test
result for that single patch is not very good.  Because without that,
the end result of the whole series will be not very good.

>> 
>> Patch 1 is a clean up patch.
>
> Could it be separated patch?
>
>> Patch 2 creates a lock per cluster, this gives us a more fine graind lock
>>         that can be used for accessing swap_map, and not lock the whole
>>         swap device
>
> I hope you make three steps to review easier. You can create some functions like
> swap_map_lock and cluster_lock which are wrapper functions just hold swap_lock.
> It doesn't change anything performance pov but it clearly shows what kinds of lock
> we should use in specific context.
>
> Then, you can introduce more fine-graind lock in next patch and apply it into
> those wrapper functions.
>
> And last patch, you can adjust cluster distribution to avoid false-sharing.
> And the description should include how it's bad in testing so it's worth.
>
> Frankly speaking, although I'm huge user of bit_spin_lock(zram/zsmalloc
> have used it heavily), I don't like swap subsystem uses it.
> During zram development, it really hurts debugging due to losing lockdep.
> The reason zram have used it is by size concern of embedded world but server
> would be not critical so please consider trade-off of spinlock vs. bit_spin_lock.

There will be one struct swap_cluster_info for every 1MB swap space.
So, for example, for 1TB swap space, the number of struct
swap_cluster_info will be one million.  To reduce the RAM usage, we
choose to use bit_spin_lock, otherwise, spinlock is better.  The code
will be used by embedded, PC and server, so the RAM usage is important.

Best Regards,
Huang, Ying

>> Patch 3 splits the swap cache radix tree into 64MB chunks, reducing
>>         the rate that we have to contende for the radix tree.
>
> To me, it's rather hacky. I think it might be common problem for page cache
> so can we think another generalized way like range_lock? Ccing Jan.
>
>> Patch 4 eliminates unnecessary page allocation for read ahead.
>
> Could it be separated patch?
>
>> Patch 5-9 create a per cpu cache of the swap slots, so we don't have
>>         to contend on the swap device to get a swap slot or to release
>>         a swap slot.  And we allocate and release the swap slots
>>         in batches for better efficiency.
>
>
> To me, idea is good although I feel the amount of code is rather huge and
> messy so it should include the number about the benefit, at least.
>
> And it might make some of patches in this patchset if we put this batching
> ahead before other patches redundant.
>
> Sorry for vague commenting. In this phase, it's really hard to review.

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

#1547811

Hi Huang,

On Wed, Dec 28, 2016 at 09:54:27AM +0800, Huang, Ying wrote:

< snip >

> > The patchset has used several techniqueus to reduce lock contention, for example,
> > batching alloc/free, fine-grained lock and cluster distribution to avoid cache
> > false-sharing. Each items has different complexity and benefits so could you
> > show the number for each step of pathchset? It would be better to include the
> > nubmer in each description. It helps how the patch is important when we consider
> > complexitiy of the patch.
> 
> One common problem of scalability optimization is that, after you have
> optimized one lock, the end result may be not very good, because another
> lock becomes heavily contended.  Similar problem occurs here, there are
> mainly two locks during swap out/in, one protects swap cache, the other
> protects swap device.  We can achieve good scalability only after having
> optimized the two locks.

Yes. You can describe that situation into the description. For example,
"with this patch, we can watch less swap_lock contention with perf but
overall performance is not good because swap cache lock still is still
contended heavily like below data so next patch will solve the problem".

It will make patch's justficiation clear.

> 
> You cannot say that one patch is not important just because the test
> result for that single patch is not very good.  Because without that,
> the end result of the whole series will be not very good.

I know that but this patchset are lack of number too much to justify
each works. You can show just raw number itself of a techniqueue
although it is not huge benefit or even worse. You can explain the reason
why it was not good, which would be enough motivation for next patch.

Number itself wouldn't be important but justfication is really crucial
to review/merge patchset and number will help it a lot in especially
MM community.

> 
> >> 
> >> Patch 1 is a clean up patch.
> >
> > Could it be separated patch?
> >
> >> Patch 2 creates a lock per cluster, this gives us a more fine graind lock
> >>         that can be used for accessing swap_map, and not lock the whole
> >>         swap device
> >
> > I hope you make three steps to review easier. You can create some functions like
> > swap_map_lock and cluster_lock which are wrapper functions just hold swap_lock.
> > It doesn't change anything performance pov but it clearly shows what kinds of lock
> > we should use in specific context.
> >
> > Then, you can introduce more fine-graind lock in next patch and apply it into
> > those wrapper functions.
> >
> > And last patch, you can adjust cluster distribution to avoid false-sharing.
> > And the description should include how it's bad in testing so it's worth.
> >
> > Frankly speaking, although I'm huge user of bit_spin_lock(zram/zsmalloc
> > have used it heavily), I don't like swap subsystem uses it.
> > During zram development, it really hurts debugging due to losing lockdep.
> > The reason zram have used it is by size concern of embedded world but server
> > would be not critical so please consider trade-off of spinlock vs. bit_spin_lock.
> 
> There will be one struct swap_cluster_info for every 1MB swap space.
> So, for example, for 1TB swap space, the number of struct
> swap_cluster_info will be one million.  To reduce the RAM usage, we
> choose to use bit_spin_lock, otherwise, spinlock is better.  The code
> will be used by embedded, PC and server, so the RAM usage is important.

It seems you already increase swap_cluster_info 4 byte to support
bit_spin_lock.
Compared to that, how much memory does spin_lock increase?

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

#1547815

Minchan Kim <minchan@kernel.org> writes:

> Hi Huang,
>
> On Wed, Dec 28, 2016 at 09:54:27AM +0800, Huang, Ying wrote:
>
> < snip >
>
>> > The patchset has used several techniqueus to reduce lock contention, for example,
>> > batching alloc/free, fine-grained lock and cluster distribution to avoid cache
>> > false-sharing. Each items has different complexity and benefits so could you
>> > show the number for each step of pathchset? It would be better to include the
>> > nubmer in each description. It helps how the patch is important when we consider
>> > complexitiy of the patch.
>> 
>> One common problem of scalability optimization is that, after you have
>> optimized one lock, the end result may be not very good, because another
>> lock becomes heavily contended.  Similar problem occurs here, there are
>> mainly two locks during swap out/in, one protects swap cache, the other
>> protects swap device.  We can achieve good scalability only after having
>> optimized the two locks.
>
> Yes. You can describe that situation into the description. For example,
> "with this patch, we can watch less swap_lock contention with perf but
> overall performance is not good because swap cache lock still is still
> contended heavily like below data so next patch will solve the problem".
>
> It will make patch's justficiation clear.
>
>> 
>> You cannot say that one patch is not important just because the test
>> result for that single patch is not very good.  Because without that,
>> the end result of the whole series will be not very good.
>
> I know that but this patchset are lack of number too much to justify
> each works. You can show just raw number itself of a techniqueue
> although it is not huge benefit or even worse. You can explain the reason
> why it was not good, which would be enough motivation for next patch.
>
> Number itself wouldn't be important but justfication is really crucial
> to review/merge patchset and number will help it a lot in especially
> MM community.
>
>> 
>> >> 
>> >> Patch 1 is a clean up patch.
>> >
>> > Could it be separated patch?
>> >
>> >> Patch 2 creates a lock per cluster, this gives us a more fine graind lock
>> >>         that can be used for accessing swap_map, and not lock the whole
>> >>         swap device
>> >
>> > I hope you make three steps to review easier. You can create some functions like
>> > swap_map_lock and cluster_lock which are wrapper functions just hold swap_lock.
>> > It doesn't change anything performance pov but it clearly shows what kinds of lock
>> > we should use in specific context.
>> >
>> > Then, you can introduce more fine-graind lock in next patch and apply it into
>> > those wrapper functions.
>> >
>> > And last patch, you can adjust cluster distribution to avoid false-sharing.
>> > And the description should include how it's bad in testing so it's worth.
>> >
>> > Frankly speaking, although I'm huge user of bit_spin_lock(zram/zsmalloc
>> > have used it heavily), I don't like swap subsystem uses it.
>> > During zram development, it really hurts debugging due to losing lockdep.
>> > The reason zram have used it is by size concern of embedded world but server
>> > would be not critical so please consider trade-off of spinlock vs. bit_spin_lock.
>> 
>> There will be one struct swap_cluster_info for every 1MB swap space.
>> So, for example, for 1TB swap space, the number of struct
>> swap_cluster_info will be one million.  To reduce the RAM usage, we
>> choose to use bit_spin_lock, otherwise, spinlock is better.  The code
>> will be used by embedded, PC and server, so the RAM usage is important.
>
> It seems you already increase swap_cluster_info 4 byte to support
> bit_spin_lock.

The increment only occurs on 64bit platform.  On 32bit platform, the
size is the same as before.

> Compared to that, how much memory does spin_lock increase?

The size of struct swap_cluster_info will increase from 4 bytes to 16
bytes on 64bit platform.  I guess it will increase from 4 bytes to 8
bytes on 32bit platform at least, but I did not test that.

Best Regards,
Huang, Ying

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

#1547819

"Huang, Ying" <ying.huang@intel.com> writes:

> Minchan Kim <minchan@kernel.org> writes:
>
>> Hi Huang,
>>
>> On Wed, Dec 28, 2016 at 09:54:27AM +0800, Huang, Ying wrote:
>>
>> < snip >
>>
>>> > The patchset has used several techniqueus to reduce lock contention, for example,
>>> > batching alloc/free, fine-grained lock and cluster distribution to avoid cache
>>> > false-sharing. Each items has different complexity and benefits so could you
>>> > show the number for each step of pathchset? It would be better to include the
>>> > nubmer in each description. It helps how the patch is important when we consider
>>> > complexitiy of the patch.
>>> 
>>> One common problem of scalability optimization is that, after you have
>>> optimized one lock, the end result may be not very good, because another
>>> lock becomes heavily contended.  Similar problem occurs here, there are
>>> mainly two locks during swap out/in, one protects swap cache, the other
>>> protects swap device.  We can achieve good scalability only after having
>>> optimized the two locks.
>>
>> Yes. You can describe that situation into the description. For example,
>> "with this patch, we can watch less swap_lock contention with perf but
>> overall performance is not good because swap cache lock still is still
>> contended heavily like below data so next patch will solve the problem".
>>
>> It will make patch's justficiation clear.
>>
>>> 
>>> You cannot say that one patch is not important just because the test
>>> result for that single patch is not very good.  Because without that,
>>> the end result of the whole series will be not very good.
>>
>> I know that but this patchset are lack of number too much to justify
>> each works. You can show just raw number itself of a techniqueue
>> although it is not huge benefit or even worse. You can explain the reason
>> why it was not good, which would be enough motivation for next patch.
>>
>> Number itself wouldn't be important but justfication is really crucial
>> to review/merge patchset and number will help it a lot in especially
>> MM community.
>>
>>> 
>>> >> 
>>> >> Patch 1 is a clean up patch.
>>> >
>>> > Could it be separated patch?
>>> >
>>> >> Patch 2 creates a lock per cluster, this gives us a more fine graind lock
>>> >>         that can be used for accessing swap_map, and not lock the whole
>>> >>         swap device
>>> >
>>> > I hope you make three steps to review easier. You can create some functions like
>>> > swap_map_lock and cluster_lock which are wrapper functions just hold swap_lock.
>>> > It doesn't change anything performance pov but it clearly shows what kinds of lock
>>> > we should use in specific context.
>>> >
>>> > Then, you can introduce more fine-graind lock in next patch and apply it into
>>> > those wrapper functions.
>>> >
>>> > And last patch, you can adjust cluster distribution to avoid false-sharing.
>>> > And the description should include how it's bad in testing so it's worth.
>>> >
>>> > Frankly speaking, although I'm huge user of bit_spin_lock(zram/zsmalloc
>>> > have used it heavily), I don't like swap subsystem uses it.
>>> > During zram development, it really hurts debugging due to losing lockdep.
>>> > The reason zram have used it is by size concern of embedded world but server
>>> > would be not critical so please consider trade-off of spinlock vs. bit_spin_lock.
>>> 
>>> There will be one struct swap_cluster_info for every 1MB swap space.
>>> So, for example, for 1TB swap space, the number of struct
>>> swap_cluster_info will be one million.  To reduce the RAM usage, we
>>> choose to use bit_spin_lock, otherwise, spinlock is better.  The code
>>> will be used by embedded, PC and server, so the RAM usage is important.
>>
>> It seems you already increase swap_cluster_info 4 byte to support
>> bit_spin_lock.
>
> The increment only occurs on 64bit platform.  On 32bit platform, the
> size is the same as before.
>
>> Compared to that, how much memory does spin_lock increase?
>
> The size of struct swap_cluster_info will increase from 4 bytes to 16
> bytes on 64bit platform.  I guess it will increase from 4 bytes to 8
> bytes on 32bit platform at least, but I did not test that.

Sorry, I make a mistake during test.  The size of struct
swap_cluster_info will increase from 4 bytes to 8 bytes on 64 bit
platform.  I think it will increase from 4 bytes to 8 bytes on 32 bit
platform too (not tested).

Best Regards,
Huang, Ying

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

#1547826

On Wed, Dec 28, 2016 at 11:31:06AM +0800, Huang, Ying wrote:

< snip >

> >>> > Frankly speaking, although I'm huge user of bit_spin_lock(zram/zsmalloc
> >>> > have used it heavily), I don't like swap subsystem uses it.
> >>> > During zram development, it really hurts debugging due to losing lockdep.
> >>> > The reason zram have used it is by size concern of embedded world but server
> >>> > would be not critical so please consider trade-off of spinlock vs. bit_spin_lock.
> >>> 
> >>> There will be one struct swap_cluster_info for every 1MB swap space.
> >>> So, for example, for 1TB swap space, the number of struct
> >>> swap_cluster_info will be one million.  To reduce the RAM usage, we
> >>> choose to use bit_spin_lock, otherwise, spinlock is better.  The code
> >>> will be used by embedded, PC and server, so the RAM usage is important.
> >>
> >> It seems you already increase swap_cluster_info 4 byte to support
> >> bit_spin_lock.
> >
> > The increment only occurs on 64bit platform.  On 32bit platform, the
> > size is the same as before.
> >
> >> Compared to that, how much memory does spin_lock increase?
> >
> > The size of struct swap_cluster_info will increase from 4 bytes to 16
> > bytes on 64bit platform.  I guess it will increase from 4 bytes to 8
> > bytes on 32bit platform at least, but I did not test that.
> 
> Sorry, I make a mistake during test.  The size of struct
> swap_cluster_info will increase from 4 bytes to 8 bytes on 64 bit
> platform.  I think it will increase from 4 bytes to 8 bytes on 32 bit
> platform too (not tested).

Thanks for the information.
To me, it's not big when we consider spinlock's usefullness which helps
cache-line bouncing, lockdep and happy with RT people.
So, I vote spin_lock but I'm not in charge of deciding on that and your
opinion might be different still. If so, let's pass the decision to
maintainer.
Instead, please write down above content in description for maintainer to
judge it fairly.

Thanks.

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

#1547832

Minchan Kim <minchan@kernel.org> writes:

> On Wed, Dec 28, 2016 at 11:31:06AM +0800, Huang, Ying wrote:
>
> < snip >
>
>> >>> > Frankly speaking, although I'm huge user of bit_spin_lock(zram/zsmalloc
>> >>> > have used it heavily), I don't like swap subsystem uses it.
>> >>> > During zram development, it really hurts debugging due to losing lockdep.
>> >>> > The reason zram have used it is by size concern of embedded world but server
>> >>> > would be not critical so please consider trade-off of spinlock vs. bit_spin_lock.
>> >>> 
>> >>> There will be one struct swap_cluster_info for every 1MB swap space.
>> >>> So, for example, for 1TB swap space, the number of struct
>> >>> swap_cluster_info will be one million.  To reduce the RAM usage, we
>> >>> choose to use bit_spin_lock, otherwise, spinlock is better.  The code
>> >>> will be used by embedded, PC and server, so the RAM usage is important.
>> >>
>> >> It seems you already increase swap_cluster_info 4 byte to support
>> >> bit_spin_lock.
>> >
>> > The increment only occurs on 64bit platform.  On 32bit platform, the
>> > size is the same as before.
>> >
>> >> Compared to that, how much memory does spin_lock increase?
>> >
>> > The size of struct swap_cluster_info will increase from 4 bytes to 16
>> > bytes on 64bit platform.  I guess it will increase from 4 bytes to 8
>> > bytes on 32bit platform at least, but I did not test that.
>> 
>> Sorry, I make a mistake during test.  The size of struct
>> swap_cluster_info will increase from 4 bytes to 8 bytes on 64 bit
>> platform.  I think it will increase from 4 bytes to 8 bytes on 32 bit
>> platform too (not tested).
>
> Thanks for the information.
> To me, it's not big when we consider spinlock's usefullness which helps
> cache-line bouncing, lockdep and happy with RT people.

Yes.  spinlock helps on lockdep and RT, but I don't think it helps
cache-line bouncing.

> So, I vote spin_lock but I'm not in charge of deciding on that and your
> opinion might be different still. If so, let's pass the decision to
> maintainer.

I have no strong opinion for size change on 32bit platform.  But I want
to know other people's opinion, especially maintainer's too.

> Instead, please write down above content in description for maintainer to
> judge it fairly.

Sure.

Best Regards,
Huang, Ying

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

#1549259

Hi,

On Tue 27-12-16 16:45:03, Minchan Kim wrote:
> > Patch 3 splits the swap cache radix tree into 64MB chunks, reducing
> >         the rate that we have to contende for the radix tree.
> 
> To me, it's rather hacky. I think it might be common problem for page cache
> so can we think another generalized way like range_lock? Ccing Jan.

I agree on the hackyness of the patch and that page cache would suffer with
the same contention (although the files are usually smaller than swap so it
would not be that visible I guess). But I don't see how range lock would
help here - we need to serialize modifications of the tree structure itself
and that is difficult to achieve with the range lock. So what you would
need is either a different data structure for tracking swap cache entries
or a finer grained locking of the radix tree.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

#1549530

Hi Jan,

On Mon, Jan 02, 2017 at 04:48:41PM +0100, Jan Kara wrote:
> Hi,
> 
> On Tue 27-12-16 16:45:03, Minchan Kim wrote:
> > > Patch 3 splits the swap cache radix tree into 64MB chunks, reducing
> > >         the rate that we have to contende for the radix tree.
> > 
> > To me, it's rather hacky. I think it might be common problem for page cache
> > so can we think another generalized way like range_lock? Ccing Jan.
> 
> I agree on the hackyness of the patch and that page cache would suffer with
> the same contention (although the files are usually smaller than swap so it
> would not be that visible I guess). But I don't see how range lock would
> help here - we need to serialize modifications of the tree structure itself
> and that is difficult to achieve with the range lock. So what you would
> need is either a different data structure for tracking swap cache entries
> or a finer grained locking of the radix tree.

Thanks for the comment, Jan.

I think there are more general options. One is to shrink batching pages like
Mel and Tim had approached.

https://patchwork.kernel.org/patch/9008421/
https://patchwork.kernel.org/patch/9322793/

Or concurrent page cache by peter.

https://www.kernel.org/doc/ols/2007/ols2007v2-pages-311-318.pdf

Ccing Nick who might have an interest on lockless page cache.

Thanks.

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

#1549544

Hi, Minchan,

Minchan Kim <minchan@kernel.org> writes:

> Hi Jan,
>
> On Mon, Jan 02, 2017 at 04:48:41PM +0100, Jan Kara wrote:
>> Hi,
>> 
>> On Tue 27-12-16 16:45:03, Minchan Kim wrote:
>> > > Patch 3 splits the swap cache radix tree into 64MB chunks, reducing
>> > >         the rate that we have to contende for the radix tree.
>> > 
>> > To me, it's rather hacky. I think it might be common problem for page cache
>> > so can we think another generalized way like range_lock? Ccing Jan.
>> 
>> I agree on the hackyness of the patch and that page cache would suffer with
>> the same contention (although the files are usually smaller than swap so it
>> would not be that visible I guess). But I don't see how range lock would
>> help here - we need to serialize modifications of the tree structure itself
>> and that is difficult to achieve with the range lock. So what you would
>> need is either a different data structure for tracking swap cache entries
>> or a finer grained locking of the radix tree.
>
> Thanks for the comment, Jan.
>
> I think there are more general options. One is to shrink batching pages like
> Mel and Tim had approached.
>
> https://patchwork.kernel.org/patch/9008421/
> https://patchwork.kernel.org/patch/9322793/

This helps to reduce the lock contention on radix tree of swap cache.
But splitting swap cache has much better performance.  So we switched
from that solution to current solution.

> Or concurrent page cache by peter.
>
> https://www.kernel.org/doc/ols/2007/ols2007v2-pages-311-318.pdf

I think this is good, it helps swap and file cache.  But I don't know
whether other people want to go this way and how much effort will be
needed.

In contrast, splitting swap cache is quite simple, for implementation
and review.  And the effect is good.

Best Regards,
Huang, Ying

> Ccing Nick who might have an interest on lockless page cache.
>
> Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web