| Message ID | 20200501015259.32237-3-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] mm/swapfile.c: classify SWAP_MAP_XXX to make it more readable | expand |
On Fri, 1 May 2020 01:52:59 +0000 Wei Yang <richard.weiyang@gmail.com> 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.
On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >On Fri, 1 May 2020 01:52:59 +0000 Wei Yang <richard.weiyang@gmail.com> 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. >
On Sat, May 02, 2020 at 01:29:11PM +0000, Wei Yang wrote: >On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >>On Fri, 1 May 2020 01:52:59 +0000 Wei Yang <richard.weiyang@gmail.com> 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 <richard.weiyang@gmail.com> writes: > On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >>On Fri, 1 May 2020 01:52:59 +0000 Wei Yang <richard.weiyang@gmail.com> 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
On Wed, May 06, 2020 at 04:22:54PM +0800, Huang, Ying wrote: >Wei Yang <richard.weiyang@gmail.com> writes: > >> On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >>>On Fri, 1 May 2020 01:52:59 +0000 Wei Yang <richard.weiyang@gmail.com> 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 <richard.weiyang@gmail.com> writes: > On Wed, May 06, 2020 at 04:22:54PM +0800, Huang, Ying wrote: >>Wei Yang <richard.weiyang@gmail.com> writes: >> >>> On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >>>>On Fri, 1 May 2020 01:52:59 +0000 Wei Yang <richard.weiyang@gmail.com> 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
On Fri, May 08, 2020 at 07:48:01AM +0800, Huang, Ying wrote: >Wei Yang <richard.weiyang@gmail.com> writes: > >> On Wed, May 06, 2020 at 04:22:54PM +0800, Huang, Ying wrote: >>>Wei Yang <richard.weiyang@gmail.com> writes: >>> >>>> On Fri, May 01, 2020 at 03:48:53PM -0700, Andrew Morton wrote: >>>>>On Fri, 1 May 2020 01:52:59 +0000 Wei Yang <richard.weiyang@gmail.com> 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
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
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 <richard.weiyang@gmail.com> --- mm/swapfile.c | 2 -- 1 file changed, 2 deletions(-)