* [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
@ 2020-04-22 21:41 Wei Yang
2020-04-23 5:57 ` Huang, Ying
0 siblings, 1 reply; 11+ messages in thread
From: Wei Yang @ 2020-04-22 21:41 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, linux-kernel, ying.huang, Wei Yang, Hugh Dickins
After commit c60aa176c6de8 ("swapfile: swap allocation cycle if
nonrot"), swap allocation is cyclic. Current approach is done with two
separate loop on the upper and lower half. This looks a little
redundant.
From another point of view, the loop iterates [lowest_bit, highest_bit]
range starting with (offset + 1) but except scan_base. So we can
simplify the loop with condition (next_offset() != scan_base) by
introducing next_offset() which makes sure offset fit in that range
with correct order.
Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
CC: Hugh Dickins <hughd@google.com>
CC: "Huang, Ying" <ying.huang@intel.com>
---
v2:
* return scan_base if the lower part is eaten
* only start over when iterating on the upper part
---
mm/swapfile.c | 31 ++++++++++++++-----------------
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index f903e5a165d5..0005a4a1c1b4 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -729,6 +729,19 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
}
}
+static unsigned long next_offset(struct swap_info_struct *si,
+ unsigned long *offset, unsigned long scan_base)
+{
+ /* only start over when iterating on the upper part */
+ if (++(*offset) > si->highest_bit && *offset > scan_base) {
+ *offset = si->lowest_bit;
+ /* someone has eaten the lower part */
+ if (si->lowest_bit >= scan_base)
+ return scan_base;
+ }
+ return *offset;
+}
+
static int scan_swap_map_slots(struct swap_info_struct *si,
unsigned char usage, int nr,
swp_entry_t slots[])
@@ -876,22 +889,7 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
scan:
spin_unlock(&si->lock);
- while (++offset <= si->highest_bit) {
- if (!si->swap_map[offset]) {
- spin_lock(&si->lock);
- goto checks;
- }
- if (vm_swap_full() && si->swap_map[offset] == SWAP_HAS_CACHE) {
- spin_lock(&si->lock);
- goto checks;
- }
- if (unlikely(--latency_ration < 0)) {
- cond_resched();
- latency_ration = LATENCY_LIMIT;
- }
- }
- offset = si->lowest_bit;
- while (offset < scan_base) {
+ while (next_offset(si, &offset, scan_base) != scan_base) {
if (!si->swap_map[offset]) {
spin_lock(&si->lock);
goto checks;
@@ -904,7 +902,6 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
cond_resched();
latency_ration = LATENCY_LIMIT;
}
- offset++;
}
spin_lock(&si->lock);
--
2.23.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-22 21:41 [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots() Wei Yang
@ 2020-04-23 5:57 ` Huang, Ying
2020-04-23 13:15 ` Wei Yang
0 siblings, 1 reply; 11+ messages in thread
From: Huang, Ying @ 2020-04-23 5:57 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, linux-mm, linux-kernel, Hugh Dickins
Wei Yang <richard.weiyang@gmail.com> writes:
> After commit c60aa176c6de8 ("swapfile: swap allocation cycle if
> nonrot"), swap allocation is cyclic. Current approach is done with two
> separate loop on the upper and lower half. This looks a little
> redundant.
I can understand that the redundant code doesn't smell good. But I
don't think the new code is easier to be understood than the original
one.
> From another point of view, the loop iterates [lowest_bit, highest_bit]
> range starting with (offset + 1) but except scan_base. So we can
> simplify the loop with condition (next_offset() != scan_base) by
> introducing next_offset() which makes sure offset fit in that range
> with correct order.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Hugh Dickins <hughd@google.com>
> CC: "Huang, Ying" <ying.huang@intel.com>
>
> ---
> v2:
> * return scan_base if the lower part is eaten
> * only start over when iterating on the upper part
> ---
> mm/swapfile.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index f903e5a165d5..0005a4a1c1b4 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -729,6 +729,19 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
> }
> }
>
> +static unsigned long next_offset(struct swap_info_struct *si,
> + unsigned long *offset, unsigned long scan_base)
> +{
> + /* only start over when iterating on the upper part */
> + if (++(*offset) > si->highest_bit && *offset > scan_base) {
> + *offset = si->lowest_bit;
> + /* someone has eaten the lower part */
> + if (si->lowest_bit >= scan_base)
> + return scan_base;
> + }
if "offset > si->highest_bit" is true and "offset < scan_base" is true,
scan_base need to be returned.
Again, the new code doesn't make it easier to find this kind of issues.
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-23 5:57 ` Huang, Ying
@ 2020-04-23 13:15 ` Wei Yang
2020-04-24 2:02 ` Huang, Ying
0 siblings, 1 reply; 11+ messages in thread
From: Wei Yang @ 2020-04-23 13:15 UTC (permalink / raw)
To: Huang, Ying; +Cc: Wei Yang, akpm, linux-mm, linux-kernel, Hugh Dickins
On Thu, Apr 23, 2020 at 01:57:34PM +0800, Huang, Ying wrote:
>Wei Yang <richard.weiyang@gmail.com> writes:
>
>> After commit c60aa176c6de8 ("swapfile: swap allocation cycle if
>> nonrot"), swap allocation is cyclic. Current approach is done with two
>> separate loop on the upper and lower half. This looks a little
>> redundant.
>
>I can understand that the redundant code doesn't smell good. But I
>don't think the new code is easier to be understood than the original
>one.
>
>> From another point of view, the loop iterates [lowest_bit, highest_bit]
>> range starting with (offset + 1) but except scan_base. So we can
>> simplify the loop with condition (next_offset() != scan_base) by
>> introducing next_offset() which makes sure offset fit in that range
>> with correct order.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Hugh Dickins <hughd@google.com>
>> CC: "Huang, Ying" <ying.huang@intel.com>
>>
>> ---
>> v2:
>> * return scan_base if the lower part is eaten
>> * only start over when iterating on the upper part
>> ---
>> mm/swapfile.c | 31 ++++++++++++++-----------------
>> 1 file changed, 14 insertions(+), 17 deletions(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index f903e5a165d5..0005a4a1c1b4 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -729,6 +729,19 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
>> }
>> }
>>
>> +static unsigned long next_offset(struct swap_info_struct *si,
>> + unsigned long *offset, unsigned long scan_base)
>> +{
>> + /* only start over when iterating on the upper part */
>> + if (++(*offset) > si->highest_bit && *offset > scan_base) {
>> + *offset = si->lowest_bit;
>> + /* someone has eaten the lower part */
>> + if (si->lowest_bit >= scan_base)
>> + return scan_base;
>> + }
>
>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>scan_base need to be returned.
>
When this case would happen in the original code?
>Again, the new code doesn't make it easier to find this kind of issues.
>
>Best Regards,
>Huang, Ying
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-23 13:15 ` Wei Yang
@ 2020-04-24 2:02 ` Huang, Ying
2020-04-25 0:30 ` Wei Yang
0 siblings, 1 reply; 11+ messages in thread
From: Huang, Ying @ 2020-04-24 2:02 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, linux-mm, linux-kernel, Hugh Dickins
Wei Yang <richard.weiyang@gmail.com> writes:
> On Thu, Apr 23, 2020 at 01:57:34PM +0800, Huang, Ying wrote:
>>Wei Yang <richard.weiyang@gmail.com> writes:
>>
>>> After commit c60aa176c6de8 ("swapfile: swap allocation cycle if
>>> nonrot"), swap allocation is cyclic. Current approach is done with two
>>> separate loop on the upper and lower half. This looks a little
>>> redundant.
>>
>>I can understand that the redundant code doesn't smell good. But I
>>don't think the new code is easier to be understood than the original
>>one.
>>
>>> From another point of view, the loop iterates [lowest_bit, highest_bit]
>>> range starting with (offset + 1) but except scan_base. So we can
>>> simplify the loop with condition (next_offset() != scan_base) by
>>> introducing next_offset() which makes sure offset fit in that range
>>> with correct order.
>>>
>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>> CC: Hugh Dickins <hughd@google.com>
>>> CC: "Huang, Ying" <ying.huang@intel.com>
>>>
>>> ---
>>> v2:
>>> * return scan_base if the lower part is eaten
>>> * only start over when iterating on the upper part
>>> ---
>>> mm/swapfile.c | 31 ++++++++++++++-----------------
>>> 1 file changed, 14 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>>> index f903e5a165d5..0005a4a1c1b4 100644
>>> --- a/mm/swapfile.c
>>> +++ b/mm/swapfile.c
>>> @@ -729,6 +729,19 @@ static void swap_range_free(struct swap_info_struct *si, unsigned long offset,
>>> }
>>> }
>>>
>>> +static unsigned long next_offset(struct swap_info_struct *si,
>>> + unsigned long *offset, unsigned long scan_base)
>>> +{
>>> + /* only start over when iterating on the upper part */
>>> + if (++(*offset) > si->highest_bit && *offset > scan_base) {
>>> + *offset = si->lowest_bit;
>>> + /* someone has eaten the lower part */
>>> + if (si->lowest_bit >= scan_base)
>>> + return scan_base;
>>> + }
>>
>>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>>scan_base need to be returned.
>>
>
> When this case would happen in the original code?
In the original code, the loop can still stop.
Best Regards,
Huang, Ying
>>Again, the new code doesn't make it easier to find this kind of issues.
>>
>>Best Regards,
>>Huang, Ying
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-24 2:02 ` Huang, Ying
@ 2020-04-25 0:30 ` Wei Yang
2020-04-26 1:07 ` Huang, Ying
0 siblings, 1 reply; 11+ messages in thread
From: Wei Yang @ 2020-04-25 0:30 UTC (permalink / raw)
To: Huang, Ying; +Cc: Wei Yang, akpm, linux-mm, linux-kernel, Hugh Dickins
On Fri, Apr 24, 2020 at 10:02:58AM +0800, Huang, Ying wrote:
>Wei Yang <richard.weiyang@gmail.com> writes:
>
[...]
>>>
>>>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>>>scan_base need to be returned.
>>>
>>
>> When this case would happen in the original code?
>
>In the original code, the loop can still stop.
>
Sorry, I don't get your point yet.
In original code, there are two separate loops
while (++offset <= si->highest_bit) {
}
while (offset < scan_base) {
}
And for your condition, (offset > highest_bit) && (offset < scan_base), which
terminates the first loop and fits the second loop well.
Not sure how this condition would stop the loop in original code?
>Best Regards,
>Huang, Ying
>
>>>Again, the new code doesn't make it easier to find this kind of issues.
>>>
>>>Best Regards,
>>>Huang, Ying
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-25 0:30 ` Wei Yang
@ 2020-04-26 1:07 ` Huang, Ying
2020-04-26 21:19 ` Wei Yang
0 siblings, 1 reply; 11+ messages in thread
From: Huang, Ying @ 2020-04-26 1:07 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, linux-mm, linux-kernel, Hugh Dickins
Wei Yang <richard.weiyang@gmail.com> writes:
> On Fri, Apr 24, 2020 at 10:02:58AM +0800, Huang, Ying wrote:
>>Wei Yang <richard.weiyang@gmail.com> writes:
>>
> [...]
>>>>
>>>>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>>>>scan_base need to be returned.
>>>>
>>>
>>> When this case would happen in the original code?
>>
>>In the original code, the loop can still stop.
>>
>
> Sorry, I don't get your point yet.
>
> In original code, there are two separate loops
>
> while (++offset <= si->highest_bit) {
> }
>
> while (offset < scan_base) {
> }
>
> And for your condition, (offset > highest_bit) && (offset < scan_base), which
> terminates the first loop and fits the second loop well.
>
> Not sure how this condition would stop the loop in original code?
Per my understanding, in your code, if some other task changes
si->highest_bit to be less than scan_base in parallel. The loop may
cannot stop.
Best Regards,
Huang, Ying
>>Best Regards,
>>Huang, Ying
>>
>>>>Again, the new code doesn't make it easier to find this kind of issues.
>>>>
>>>>Best Regards,
>>>>Huang, Ying
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-26 1:07 ` Huang, Ying
@ 2020-04-26 21:19 ` Wei Yang
2020-04-27 0:55 ` Huang, Ying
0 siblings, 1 reply; 11+ messages in thread
From: Wei Yang @ 2020-04-26 21:19 UTC (permalink / raw)
To: Huang, Ying; +Cc: Wei Yang, akpm, linux-mm, linux-kernel, Hugh Dickins
On Sun, Apr 26, 2020 at 09:07:11AM +0800, Huang, Ying wrote:
>Wei Yang <richard.weiyang@gmail.com> writes:
>
>> On Fri, Apr 24, 2020 at 10:02:58AM +0800, Huang, Ying wrote:
>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>
>> [...]
>>>>>
>>>>>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>>>>>scan_base need to be returned.
>>>>>
>>>>
>>>> When this case would happen in the original code?
>>>
>>>In the original code, the loop can still stop.
>>>
>>
>> Sorry, I don't get your point yet.
>>
>> In original code, there are two separate loops
>>
>> while (++offset <= si->highest_bit) {
>> }
>>
>> while (offset < scan_base) {
>> }
>>
>> And for your condition, (offset > highest_bit) && (offset < scan_base), which
>> terminates the first loop and fits the second loop well.
>>
>> Not sure how this condition would stop the loop in original code?
>
>Per my understanding, in your code, if some other task changes
>si->highest_bit to be less than scan_base in parallel. The loop may
>cannot stop.
When (offset > scan_base), (offset > si->highest_bit) means offset will be
set to si->lowest_bit.
When (offset < scan_base), next_offset() would always increase offset till
offset is scan_base.
Sorry, I didn't catch your case. Would you minding giving more detail?
>
>Best Regards,
>Huang, Ying
>
>>>Best Regards,
>>>Huang, Ying
>>>
>>>>>Again, the new code doesn't make it easier to find this kind of issues.
>>>>>
>>>>>Best Regards,
>>>>>Huang, Ying
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-26 21:19 ` Wei Yang
@ 2020-04-27 0:55 ` Huang, Ying
2020-04-28 21:22 ` Wei Yang
0 siblings, 1 reply; 11+ messages in thread
From: Huang, Ying @ 2020-04-27 0:55 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, linux-mm, linux-kernel, Hugh Dickins
Wei Yang <richard.weiyang@gmail.com> writes:
> On Sun, Apr 26, 2020 at 09:07:11AM +0800, Huang, Ying wrote:
>>Wei Yang <richard.weiyang@gmail.com> writes:
>>
>>> On Fri, Apr 24, 2020 at 10:02:58AM +0800, Huang, Ying wrote:
>>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>>
>>> [...]
>>>>>>
>>>>>>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>>>>>>scan_base need to be returned.
>>>>>>
>>>>>
>>>>> When this case would happen in the original code?
>>>>
>>>>In the original code, the loop can still stop.
>>>>
>>>
>>> Sorry, I don't get your point yet.
>>>
>>> In original code, there are two separate loops
>>>
>>> while (++offset <= si->highest_bit) {
>>> }
>>>
>>> while (offset < scan_base) {
>>> }
>>>
>>> And for your condition, (offset > highest_bit) && (offset < scan_base), which
>>> terminates the first loop and fits the second loop well.
>>>
>>> Not sure how this condition would stop the loop in original code?
>>
>>Per my understanding, in your code, if some other task changes
>>si->highest_bit to be less than scan_base in parallel. The loop may
>>cannot stop.
>
> When (offset > scan_base), (offset > si->highest_bit) means offset will be
> set to si->lowest_bit.
>
> When (offset < scan_base), next_offset() would always increase offset till
> offset is scan_base.
>
> Sorry, I didn't catch your case. Would you minding giving more detail?
Don't think in single thread model. There's no lock to prevent other
tasks to change si->highest_bit simultaneously. For example, task B may
change si->highest_bit to be less than scan_base in task A.
Best Regards,
Huang, Ying
>>
>>Best Regards,
>>Huang, Ying
>>
>>>>Best Regards,
>>>>Huang, Ying
>>>>
>>>>>>Again, the new code doesn't make it easier to find this kind of issues.
>>>>>>
>>>>>>Best Regards,
>>>>>>Huang, Ying
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-27 0:55 ` Huang, Ying
@ 2020-04-28 21:22 ` Wei Yang
2020-04-29 0:52 ` Huang, Ying
0 siblings, 1 reply; 11+ messages in thread
From: Wei Yang @ 2020-04-28 21:22 UTC (permalink / raw)
To: Huang, Ying; +Cc: Wei Yang, akpm, linux-mm, linux-kernel, Hugh Dickins
On Mon, Apr 27, 2020 at 08:55:33AM +0800, Huang, Ying wrote:
>Wei Yang <richard.weiyang@gmail.com> writes:
>
>> On Sun, Apr 26, 2020 at 09:07:11AM +0800, Huang, Ying wrote:
>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>
>>>> On Fri, Apr 24, 2020 at 10:02:58AM +0800, Huang, Ying wrote:
>>>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>>>
>>>> [...]
>>>>>>>
>>>>>>>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>>>>>>>scan_base need to be returned.
>>>>>>>
>>>>>>
>>>>>> When this case would happen in the original code?
>>>>>
>>>>>In the original code, the loop can still stop.
>>>>>
>>>>
>>>> Sorry, I don't get your point yet.
>>>>
>>>> In original code, there are two separate loops
>>>>
>>>> while (++offset <= si->highest_bit) {
>>>> }
>>>>
>>>> while (offset < scan_base) {
>>>> }
>>>>
>>>> And for your condition, (offset > highest_bit) && (offset < scan_base), which
>>>> terminates the first loop and fits the second loop well.
>>>>
>>>> Not sure how this condition would stop the loop in original code?
>>>
>>>Per my understanding, in your code, if some other task changes
>>>si->highest_bit to be less than scan_base in parallel. The loop may
>>>cannot stop.
>>
>> When (offset > scan_base), (offset > si->highest_bit) means offset will be
>> set to si->lowest_bit.
>>
>> When (offset < scan_base), next_offset() would always increase offset till
>> offset is scan_base.
>>
>> Sorry, I didn't catch your case. Would you minding giving more detail?
>
>Don't think in single thread model. There's no lock to prevent other
>tasks to change si->highest_bit simultaneously. For example, task B may
>change si->highest_bit to be less than scan_base in task A.
>
Yes, I am trying to think about it in parallel mode.
Here are the cases, it might happen in parallel when task B change highest_bit
to be less than scan_base.
(1)
offset
v
+-------------------+------------------+
^ ^ ^
lowest_bit highest_bit scan_base
(2)
offset
v
+-------------------+------------------+
^ ^ ^
lowest_bit highest_bit scan_base
(3)
offset
v
+-------------------+------------------+
^ ^ ^
lowest_bit highest_bit scan_base
Case (1), (offset > highest) && (offset > scan_base), offset would be set to
lowest_bit. This looks good.
Case (2), (offset > highest) && (offset < scan_base), since offset is less
than scan_base, it wouldn't be set to lowest. Instead it will continue to
scan_base.
Case (3), almost the same as Case (2).
In Case (2) and (3), one thing interesting is the loop won't stop at
highest_bit, while the behavior is the same as original code.
Maybe your concern is this one? I still not figure out your point about the
infinite loop. Hope you would share some light on it.
>Best Regards,
>Huang, Ying
>
>>>
>>>Best Regards,
>>>Huang, Ying
>>>
>>>>>Best Regards,
>>>>>Huang, Ying
>>>>>
>>>>>>>Again, the new code doesn't make it easier to find this kind of issues.
>>>>>>>
>>>>>>>Best Regards,
>>>>>>>Huang, Ying
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-28 21:22 ` Wei Yang
@ 2020-04-29 0:52 ` Huang, Ying
2020-04-29 22:06 ` Wei Yang
0 siblings, 1 reply; 11+ messages in thread
From: Huang, Ying @ 2020-04-29 0:52 UTC (permalink / raw)
To: Wei Yang; +Cc: akpm, linux-mm, linux-kernel, Hugh Dickins
Wei Yang <richard.weiyang@gmail.com> writes:
> On Mon, Apr 27, 2020 at 08:55:33AM +0800, Huang, Ying wrote:
>>Wei Yang <richard.weiyang@gmail.com> writes:
>>
>>> On Sun, Apr 26, 2020 at 09:07:11AM +0800, Huang, Ying wrote:
>>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>>
>>>>> On Fri, Apr 24, 2020 at 10:02:58AM +0800, Huang, Ying wrote:
>>>>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>>>>
>>>>> [...]
>>>>>>>>
>>>>>>>>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>>>>>>>>scan_base need to be returned.
>>>>>>>>
>>>>>>>
>>>>>>> When this case would happen in the original code?
>>>>>>
>>>>>>In the original code, the loop can still stop.
>>>>>>
>>>>>
>>>>> Sorry, I don't get your point yet.
>>>>>
>>>>> In original code, there are two separate loops
>>>>>
>>>>> while (++offset <= si->highest_bit) {
>>>>> }
>>>>>
>>>>> while (offset < scan_base) {
>>>>> }
>>>>>
>>>>> And for your condition, (offset > highest_bit) && (offset < scan_base), which
>>>>> terminates the first loop and fits the second loop well.
>>>>>
>>>>> Not sure how this condition would stop the loop in original code?
>>>>
>>>>Per my understanding, in your code, if some other task changes
>>>>si->highest_bit to be less than scan_base in parallel. The loop may
>>>>cannot stop.
>>>
>>> When (offset > scan_base), (offset > si->highest_bit) means offset will be
>>> set to si->lowest_bit.
>>>
>>> When (offset < scan_base), next_offset() would always increase offset till
>>> offset is scan_base.
>>>
>>> Sorry, I didn't catch your case. Would you minding giving more detail?
>>
>>Don't think in single thread model. There's no lock to prevent other
>>tasks to change si->highest_bit simultaneously. For example, task B may
>>change si->highest_bit to be less than scan_base in task A.
>>
>
> Yes, I am trying to think about it in parallel mode.
>
> Here are the cases, it might happen in parallel when task B change highest_bit
> to be less than scan_base.
>
> (1)
> offset
> v
> +-------------------+------------------+
> ^ ^ ^
> lowest_bit highest_bit scan_base
>
>
> (2)
> offset
> v
> +-------------------+------------------+
> ^ ^ ^
> lowest_bit highest_bit scan_base
>
This is the case in my mind. But my original understanding to your code
wasn't correct. As you said, loop can stop because offset is kept
increasing. Sorry about that.
But I still don't like your new code. It's not as obvious as the
original one.
Best Regards,
Huang, Ying
> (3)
> offset
> v
> +-------------------+------------------+
> ^ ^ ^
> lowest_bit highest_bit scan_base
>
> Case (1), (offset > highest) && (offset > scan_base), offset would be set to
> lowest_bit. This looks good.
>
> Case (2), (offset > highest) && (offset < scan_base), since offset is less
> than scan_base, it wouldn't be set to lowest. Instead it will continue to
> scan_base.
>
> Case (3), almost the same as Case (2).
>
> In Case (2) and (3), one thing interesting is the loop won't stop at
> highest_bit, while the behavior is the same as original code.
>
> Maybe your concern is this one? I still not figure out your point about the
> infinite loop. Hope you would share some light on it.
>
>
>>Best Regards,
>>Huang, Ying
>>
>>>>
>>>>Best Regards,
>>>>Huang, Ying
>>>>
>>>>>>Best Regards,
>>>>>>Huang, Ying
>>>>>>
>>>>>>>>Again, the new code doesn't make it easier to find this kind of issues.
>>>>>>>>
>>>>>>>>Best Regards,
>>>>>>>>Huang, Ying
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots()
2020-04-29 0:52 ` Huang, Ying
@ 2020-04-29 22:06 ` Wei Yang
0 siblings, 0 replies; 11+ messages in thread
From: Wei Yang @ 2020-04-29 22:06 UTC (permalink / raw)
To: Huang, Ying; +Cc: Wei Yang, akpm, linux-mm, linux-kernel, Hugh Dickins
On Wed, Apr 29, 2020 at 08:52:44AM +0800, Huang, Ying wrote:
>Wei Yang <richard.weiyang@gmail.com> writes:
>
>> On Mon, Apr 27, 2020 at 08:55:33AM +0800, Huang, Ying wrote:
>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>
>>>> On Sun, Apr 26, 2020 at 09:07:11AM +0800, Huang, Ying wrote:
>>>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>>>
>>>>>> On Fri, Apr 24, 2020 at 10:02:58AM +0800, Huang, Ying wrote:
>>>>>>>Wei Yang <richard.weiyang@gmail.com> writes:
>>>>>>>
>>>>>> [...]
>>>>>>>>>
>>>>>>>>>if "offset > si->highest_bit" is true and "offset < scan_base" is true,
>>>>>>>>>scan_base need to be returned.
>>>>>>>>>
>>>>>>>>
>>>>>>>> When this case would happen in the original code?
>>>>>>>
>>>>>>>In the original code, the loop can still stop.
>>>>>>>
>>>>>>
>>>>>> Sorry, I don't get your point yet.
>>>>>>
>>>>>> In original code, there are two separate loops
>>>>>>
>>>>>> while (++offset <= si->highest_bit) {
>>>>>> }
>>>>>>
>>>>>> while (offset < scan_base) {
>>>>>> }
>>>>>>
>>>>>> And for your condition, (offset > highest_bit) && (offset < scan_base), which
>>>>>> terminates the first loop and fits the second loop well.
>>>>>>
>>>>>> Not sure how this condition would stop the loop in original code?
>>>>>
>>>>>Per my understanding, in your code, if some other task changes
>>>>>si->highest_bit to be less than scan_base in parallel. The loop may
>>>>>cannot stop.
>>>>
>>>> When (offset > scan_base), (offset > si->highest_bit) means offset will be
>>>> set to si->lowest_bit.
>>>>
>>>> When (offset < scan_base), next_offset() would always increase offset till
>>>> offset is scan_base.
>>>>
>>>> Sorry, I didn't catch your case. Would you minding giving more detail?
>>>
>>>Don't think in single thread model. There's no lock to prevent other
>>>tasks to change si->highest_bit simultaneously. For example, task B may
>>>change si->highest_bit to be less than scan_base in task A.
>>>
>>
>> Yes, I am trying to think about it in parallel mode.
>>
>> Here are the cases, it might happen in parallel when task B change highest_bit
>> to be less than scan_base.
>>
>> (1)
>> offset
>> v
>> +-------------------+------------------+
>> ^ ^ ^
>> lowest_bit highest_bit scan_base
>>
>>
>> (2)
>> offset
>> v
>> +-------------------+------------------+
>> ^ ^ ^
>> lowest_bit highest_bit scan_base
>>
>
>This is the case in my mind. But my original understanding to your code
>wasn't correct. As you said, loop can stop because offset is kept
>increasing. Sorry about that.
>
NP.
>But I still don't like your new code. It's not as obvious as the
>original one.
Sure, thanks for your time.
>
>Best Regards,
>Huang, Ying
>
>> (3)
>> offset
>> v
>> +-------------------+------------------+
>> ^ ^ ^
>> lowest_bit highest_bit scan_base
>>
>> Case (1), (offset > highest) && (offset > scan_base), offset would be set to
>> lowest_bit. This looks good.
>>
>> Case (2), (offset > highest) && (offset < scan_base), since offset is less
>> than scan_base, it wouldn't be set to lowest. Instead it will continue to
>> scan_base.
>>
>> Case (3), almost the same as Case (2).
>>
>> In Case (2) and (3), one thing interesting is the loop won't stop at
>> highest_bit, while the behavior is the same as original code.
>>
>> Maybe your concern is this one? I still not figure out your point about the
>> infinite loop. Hope you would share some light on it.
>>
>>
>>>Best Regards,
>>>Huang, Ying
>>>
>>>>>
>>>>>Best Regards,
>>>>>Huang, Ying
>>>>>
>>>>>>>Best Regards,
>>>>>>>Huang, Ying
>>>>>>>
>>>>>>>>>Again, the new code doesn't make it easier to find this kind of issues.
>>>>>>>>>
>>>>>>>>>Best Regards,
>>>>>>>>>Huang, Ying
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-29 22:06 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-22 21:41 [PATCH v2] mm/swapfile.c: simplify the scan loop in scan_swap_map_slots() Wei Yang
2020-04-23 5:57 ` Huang, Ying
2020-04-23 13:15 ` Wei Yang
2020-04-24 2:02 ` Huang, Ying
2020-04-25 0:30 ` Wei Yang
2020-04-26 1:07 ` Huang, Ying
2020-04-26 21:19 ` Wei Yang
2020-04-27 0:55 ` Huang, Ying
2020-04-28 21:22 ` Wei Yang
2020-04-29 0:52 ` Huang, Ying
2020-04-29 22:06 ` Wei Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).