Re: [PATCH 1/6] swap: Add comments to lock_cluster_or_swap_info()
Dave Hansen writes: >> +/* >> + * At most times, fine grained cluster lock is sufficient to protect > > Can we call out those times, please? To protect si->swap_map[], if HDD, si->lock is used, otherwise cluster lock is used. "at most times" is ambiguous here, I will fix it. >> + * the operations on sis->swap_map. > > Please be careful with the naming. You can call it 'si' because that's > what the function argument is named. Or, swap_info_struct because > that's the struct name. Calling it 'sis' is a bit sloppy, no? > >> No need to acquire gross grained > > "coarse" is a conventional antonym for "fine". Sorry for my poor English, will change this. >> + * sis->lock. But cluster and cluster lock isn't available for HDD, >> + * so sis->lock will be instead for them. >> + */ >> static inline struct swap_cluster_info *lock_cluster_or_swap_info( >> struct swap_info_struct *si, >> unsigned long offset) > > What I already knew was: there are two locks. We use one sometimes and > the other at other times. > > What I don't know is why there are two locks, and the heuristics why we > choose between them. This comment doesn't help explain the things I > don't know. cluster lock is used to protect fields of struct swap_cluster_info, and si->swap_map[], this is described in comments of struct swap_cluster_info. si->lock is used to protect other fields of si. If two locks need to be held, hold si->lock first. This is for non-HDD. For HDD, there are no cluster, so si->lock is used to protect si->swap_map[]. Best Regards, Huang, Ying
Re: [PATCH 1/6] swap: Add comments to lock_cluster_or_swap_info()
Dave Hansen writes: >> +/* >> + * At most times, fine grained cluster lock is sufficient to protect > > Can we call out those times, please? To protect si->swap_map[], if HDD, si->lock is used, otherwise cluster lock is used. "at most times" is ambiguous here, I will fix it. >> + * the operations on sis->swap_map. > > Please be careful with the naming. You can call it 'si' because that's > what the function argument is named. Or, swap_info_struct because > that's the struct name. Calling it 'sis' is a bit sloppy, no? > >> No need to acquire gross grained > > "coarse" is a conventional antonym for "fine". Sorry for my poor English, will change this. >> + * sis->lock. But cluster and cluster lock isn't available for HDD, >> + * so sis->lock will be instead for them. >> + */ >> static inline struct swap_cluster_info *lock_cluster_or_swap_info( >> struct swap_info_struct *si, >> unsigned long offset) > > What I already knew was: there are two locks. We use one sometimes and > the other at other times. > > What I don't know is why there are two locks, and the heuristics why we > choose between them. This comment doesn't help explain the things I > don't know. cluster lock is used to protect fields of struct swap_cluster_info, and si->swap_map[], this is described in comments of struct swap_cluster_info. si->lock is used to protect other fields of si. If two locks need to be held, hold si->lock first. This is for non-HDD. For HDD, there are no cluster, so si->lock is used to protect si->swap_map[]. Best Regards, Huang, Ying
Re: [PATCH 1/6] swap: Add comments to lock_cluster_or_swap_info()
> +/* > + * At most times, fine grained cluster lock is sufficient to protect Can we call out those times, please? > + * the operations on sis->swap_map. Please be careful with the naming. You can call it 'si' because that's what the function argument is named. Or, swap_info_struct because that's the struct name. Calling it 'sis' is a bit sloppy, no? > No need to acquire gross grained "coarse" is a conventional antonym for "fine". > + * sis->lock. But cluster and cluster lock isn't available for HDD, > + * so sis->lock will be instead for them. > + */ > static inline struct swap_cluster_info *lock_cluster_or_swap_info( > struct swap_info_struct *si, > unsigned long offset) What I already knew was: there are two locks. We use one sometimes and the other at other times. What I don't know is why there are two locks, and the heuristics why we choose between them. This comment doesn't help explain the things I don't know.
Re: [PATCH 1/6] swap: Add comments to lock_cluster_or_swap_info()
> +/* > + * At most times, fine grained cluster lock is sufficient to protect Can we call out those times, please? > + * the operations on sis->swap_map. Please be careful with the naming. You can call it 'si' because that's what the function argument is named. Or, swap_info_struct because that's the struct name. Calling it 'sis' is a bit sloppy, no? > No need to acquire gross grained "coarse" is a conventional antonym for "fine". > + * sis->lock. But cluster and cluster lock isn't available for HDD, > + * so sis->lock will be instead for them. > + */ > static inline struct swap_cluster_info *lock_cluster_or_swap_info( > struct swap_info_struct *si, > unsigned long offset) What I already knew was: there are two locks. We use one sometimes and the other at other times. What I don't know is why there are two locks, and the heuristics why we choose between them. This comment doesn't help explain the things I don't know.
[PATCH 1/6] swap: Add comments to lock_cluster_or_swap_info()
From: Huang Ying To improve the code readability. Signed-off-by: "Huang, Ying" Suggested-by: Dave Hansen Cc: Michal Hocko Cc: Johannes Weiner Cc: Shaohua Li Cc: Hugh Dickins Cc: Minchan Kim Cc: Rik van Riel Cc: Daniel Jordan Cc: Dan Williams --- mm/swapfile.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/mm/swapfile.c b/mm/swapfile.c index d8fddfb000ec..e31aa601d9c0 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -297,6 +297,12 @@ static inline void unlock_cluster(struct swap_cluster_info *ci) spin_unlock(>lock); } +/* + * At most times, fine grained cluster lock is sufficient to protect + * the operations on sis->swap_map. No need to acquire gross grained + * sis->lock. But cluster and cluster lock isn't available for HDD, + * so sis->lock will be instead for them. + */ static inline struct swap_cluster_info *lock_cluster_or_swap_info( struct swap_info_struct *si, unsigned long offset) -- 2.16.4
[PATCH 1/6] swap: Add comments to lock_cluster_or_swap_info()
From: Huang Ying To improve the code readability. Signed-off-by: "Huang, Ying" Suggested-by: Dave Hansen Cc: Michal Hocko Cc: Johannes Weiner Cc: Shaohua Li Cc: Hugh Dickins Cc: Minchan Kim Cc: Rik van Riel Cc: Daniel Jordan Cc: Dan Williams --- mm/swapfile.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/mm/swapfile.c b/mm/swapfile.c index d8fddfb000ec..e31aa601d9c0 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -297,6 +297,12 @@ static inline void unlock_cluster(struct swap_cluster_info *ci) spin_unlock(>lock); } +/* + * At most times, fine grained cluster lock is sufficient to protect + * the operations on sis->swap_map. No need to acquire gross grained + * sis->lock. But cluster and cluster lock isn't available for HDD, + * so sis->lock will be instead for them. + */ static inline struct swap_cluster_info *lock_cluster_or_swap_info( struct swap_info_struct *si, unsigned long offset) -- 2.16.4
