Re: [PATCH 3/3] mm/swapfile.c: count won't be bigger than SWAP_MAP_MAX
On Fri, May 08, 2020 at 07:48:01AM +0800, Huang, Ying wrote: >Wei Yang writes: > >> On Wed, May 06, 2020 at 04:22:54PM +0800, Huang, Ying wrote: >>>Wei Yang writes: >>> On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >On Fri, 1 May 2020 01:52:59 + Wei Yang >wrote: > >> When the condition is true, there are two possibilities: > >I'm struggling with this one. > >>1. count == SWAP_MAP_BAD >>2. count == (SWAP_MAP_MAX & COUNT_CONTINUED) == SWAP_MAP_SHMEM > >I'm not sure what 2. is trying to say. For a start, (SWAP_MAP_MAX & >COUNT_CONTINUED) is zero. I guess it meant "|"? Oops, you are right. It should be (SWAP_MAP_MAX | COUNT_CONTINUED). Sorry for the confusion. > >Also, the return value documentation says we return EINVAL for migration >entries. Where's that happening, or is the comment out of date? > Not paid attention to this. Take look into the code, I don't find a relationship between the swap count and migration. Seems we just make a migration entry but not duplicate it. If my understanding is correct. >>> >>>Per my understanding, one functionality of the error path is to catch >>>the behavior that shouldn't happen at all. For example, if >>>__swap_duplicate() is called for the migration entry because of some >>>race condition. >>> >> >> If __swap_duplicate() run for a migration entry, it returns since >> get_swap_entry() couldn't find a swap_info_struct. So the return value is >> -EINVAL. >> >> While when this situation would happen? And the race condition you mean is? > >Sorry for confusing. I don't mean there are some known race conditions >in current kernel that will trigger the error code path. I mean we may >use the error path to identify some race conditions in the future. > Yep, NP. For the code itself, do you have some comment? >I remember that Matthew thought that the swap code should work >reasonably even for garbage PTE. > >Best Regards, >Huang, Ying > >>>Best Regards, >>>Huang, Ying -- Wei Yang Help you, Help me
Re: [PATCH 3/3] mm/swapfile.c: count won't be bigger than SWAP_MAP_MAX
Wei Yang writes: > On Wed, May 06, 2020 at 04:22:54PM +0800, Huang, Ying wrote: >>Wei Yang writes: >> >>> On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: On Fri, 1 May 2020 01:52:59 + Wei Yang wrote: > When the condition is true, there are two possibilities: I'm struggling with this one. >1. count == SWAP_MAP_BAD >2. count == (SWAP_MAP_MAX & COUNT_CONTINUED) == SWAP_MAP_SHMEM I'm not sure what 2. is trying to say. For a start, (SWAP_MAP_MAX & COUNT_CONTINUED) is zero. I guess it meant "|"? >>> >>> Oops, you are right. It should be (SWAP_MAP_MAX | COUNT_CONTINUED). >>> >>> Sorry for the confusion. >>> Also, the return value documentation says we return EINVAL for migration entries. Where's that happening, or is the comment out of date? >>> >>> Not paid attention to this. >>> >>> Take look into the code, I don't find a relationship between the swap count >>> and migration. Seems we just make a migration entry but not duplicate it. >>> If my understanding is correct. >> >>Per my understanding, one functionality of the error path is to catch >>the behavior that shouldn't happen at all. For example, if >>__swap_duplicate() is called for the migration entry because of some >>race condition. >> > > If __swap_duplicate() run for a migration entry, it returns since > get_swap_entry() couldn't find a swap_info_struct. So the return value is > -EINVAL. > > While when this situation would happen? And the race condition you mean is? Sorry for confusing. I don't mean there are some known race conditions in current kernel that will trigger the error code path. I mean we may use the error path to identify some race conditions in the future. I remember that Matthew thought that the swap code should work reasonably even for garbage PTE. Best Regards, Huang, Ying >>Best Regards, >>Huang, Ying
Re: [PATCH 3/3] mm/swapfile.c: count won't be bigger than SWAP_MAP_MAX
On Wed, May 06, 2020 at 04:22:54PM +0800, Huang, Ying wrote: >Wei Yang writes: > >> On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >>>On Fri, 1 May 2020 01:52:59 + Wei Yang >>>wrote: >>> When the condition is true, there are two possibilities: >>> >>>I'm struggling with this one. >>> 1. count == SWAP_MAP_BAD 2. count == (SWAP_MAP_MAX & COUNT_CONTINUED) == SWAP_MAP_SHMEM >>> >>>I'm not sure what 2. is trying to say. For a start, (SWAP_MAP_MAX & >>>COUNT_CONTINUED) is zero. I guess it meant "|"? >> >> Oops, you are right. It should be (SWAP_MAP_MAX | COUNT_CONTINUED). >> >> Sorry for the confusion. >> >>> >>>Also, the return value documentation says we return EINVAL for migration >>>entries. Where's that happening, or is the comment out of date? >>> >> >> Not paid attention to this. >> >> Take look into the code, I don't find a relationship between the swap count >> and migration. Seems we just make a migration entry but not duplicate it. >> If my understanding is correct. > >Per my understanding, one functionality of the error path is to catch >the behavior that shouldn't happen at all. For example, if >__swap_duplicate() is called for the migration entry because of some >race condition. > If __swap_duplicate() run for a migration entry, it returns since get_swap_entry() couldn't find a swap_info_struct. So the return value is -EINVAL. While when this situation would happen? And the race condition you mean is? >Best Regards, >Huang, Ying -- Wei Yang Help you, Help me
Re: [PATCH 3/3] mm/swapfile.c: count won't be bigger than SWAP_MAP_MAX
Wei Yang writes: > On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >>On Fri, 1 May 2020 01:52:59 + Wei Yang wrote: >> >>> When the condition is true, there are two possibilities: >> >>I'm struggling with this one. >> >>>1. count == SWAP_MAP_BAD >>>2. count == (SWAP_MAP_MAX & COUNT_CONTINUED) == SWAP_MAP_SHMEM >> >>I'm not sure what 2. is trying to say. For a start, (SWAP_MAP_MAX & >>COUNT_CONTINUED) is zero. I guess it meant "|"? > > Oops, you are right. It should be (SWAP_MAP_MAX | COUNT_CONTINUED). > > Sorry for the confusion. > >> >>Also, the return value documentation says we return EINVAL for migration >>entries. Where's that happening, or is the comment out of date? >> > > Not paid attention to this. > > Take look into the code, I don't find a relationship between the swap count > and migration. Seems we just make a migration entry but not duplicate it. > If my understanding is correct. Per my understanding, one functionality of the error path is to catch the behavior that shouldn't happen at all. For example, if __swap_duplicate() is called for the migration entry because of some race condition. Best Regards, Huang, Ying
Re: [PATCH 3/3] mm/swapfile.c: count won't be bigger than SWAP_MAP_MAX
On Sat, May 02, 2020 at 01:29:11PM +, Wei Yang wrote: >On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >>On Fri, 1 May 2020 01:52:59 + Wei Yang wrote: >> >>> When the condition is true, there are two possibilities: >> >>I'm struggling with this one. >> >>>1. count == SWAP_MAP_BAD >>>2. count == (SWAP_MAP_MAX & COUNT_CONTINUED) == SWAP_MAP_SHMEM >> >>I'm not sure what 2. is trying to say. For a start, (SWAP_MAP_MAX & >>COUNT_CONTINUED) is zero. I guess it meant "|"? > >Oops, you are right. It should be (SWAP_MAP_MAX | COUNT_CONTINUED). > >Sorry for the confusion. > Hmm... I made a mistake again, the two cases should be * SWAP_MAP_BAD * (SWAP_MAP_BAD | COUNT_CONTINUED) == SWAP_MAP_SHMEM What a shame. >> >>Also, the return value documentation says we return EINVAL for migration >>entries. Where's that happening, or is the comment out of date? >> > >Not paid attention to this. > >Take look into the code, I don't find a relationship between the swap count >and migration. Seems we just make a migration entry but not duplicate it. >If my understanding is correct. > >>> The first case would be filtered by the first if in __swap_duplicate(). >>> >>> And the second case means this swap entry is for shmem. Since we never >>> do another duplication for shmem swap entry. This won't happen neither. >> > >-- >Wei Yang >Help you, Help me -- Wei Yang Help you, Help me
Re: [PATCH 3/3] mm/swapfile.c: count won't be bigger than SWAP_MAP_MAX
On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >On Fri, 1 May 2020 01:52:59 + Wei Yang wrote: > >> When the condition is true, there are two possibilities: > >I'm struggling with this one. > >>1. count == SWAP_MAP_BAD >>2. count == (SWAP_MAP_MAX & COUNT_CONTINUED) == SWAP_MAP_SHMEM > >I'm not sure what 2. is trying to say. For a start, (SWAP_MAP_MAX & >COUNT_CONTINUED) is zero. I guess it meant "|"? Oops, you are right. It should be (SWAP_MAP_MAX | COUNT_CONTINUED). Sorry for the confusion. > >Also, the return value documentation says we return EINVAL for migration >entries. Where's that happening, or is the comment out of date? > Not paid attention to this. Take look into the code, I don't find a relationship between the swap count and migration. Seems we just make a migration entry but not duplicate it. If my understanding is correct. >> The first case would be filtered by the first if in __swap_duplicate(). >> >> And the second case means this swap entry is for shmem. Since we never >> do another duplication for shmem swap entry. This won't happen neither. > -- Wei Yang Help you, Help me
Re: [PATCH 3/3] mm/swapfile.c: count won't be bigger than SWAP_MAP_MAX
On Fri, 1 May 2020 01:52:59 + Wei Yang wrote: > When the condition is true, there are two possibilities: I'm struggling with this one. >1. count == SWAP_MAP_BAD >2. count == (SWAP_MAP_MAX & COUNT_CONTINUED) == SWAP_MAP_SHMEM I'm not sure what 2. is trying to say. For a start, (SWAP_MAP_MAX & COUNT_CONTINUED) is zero. I guess it meant "|"? Also, the return value documentation says we return EINVAL for migration entries. Where's that happening, or is the comment out of date? > The first case would be filtered by the first if in __swap_duplicate(). > > And the second case means this swap entry is for shmem. Since we never > do another duplication for shmem swap entry. This won't happen neither.
[PATCH 3/3] mm/swapfile.c: count won't be bigger than SWAP_MAP_MAX
When the condition is true, there are two possibilities: 1. count == SWAP_MAP_BAD 2. count == (SWAP_MAP_MAX & COUNT_CONTINUED) == SWAP_MAP_SHMEM The first case would be filtered by the first if in __swap_duplicate(). And the second case means this swap entry is for shmem. Since we never do another duplication for shmem swap entry. This won't happen neither. Signed-off-by: Wei Yang --- mm/swapfile.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/mm/swapfile.c b/mm/swapfile.c index 1a877d1d40e3..88dd2ad34aad 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -3404,8 +3404,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage) if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX) count += usage; - else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) - err = -EINVAL; else if (swap_count_continued(p, offset, count)) count = COUNT_CONTINUED; else -- 2.23.0
