| Message ID | 20200612192618.32579-1-rcampbell@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hmm: remove redundant check non_swap_entry() | expand |
On Fri, Jun 12, 2020 at 12:26:18PM -0700, Ralph Campbell wrote: > In zap_pte_range(), the check for non_swap_entry() and > is_device_private_entry() is redundant since the latter is a subset of the > former. Remove the redundant check to simplify the code and for clarity. > > Signed-off-by: Ralph Campbell <rcampbell@nvidia.com> > > This is based on the current linux tree and is intended for Andrew's mm > tree. There is no rush so it could go into 5.9 but I think it is safe > enough to go into an rc after the patch is reviewed. Probably shouldn't mark it as mm/hmm if Andrew is going to pick it Reviewed-by: Jason Gunthorpe <jgg@mellanox.com> Jason
On Fri, Jun 12, 2020 at 12:26:18PM -0700, Ralph Campbell wrote: > In zap_pte_range(), the check for non_swap_entry() and > is_device_private_entry() is redundant since the latter is a subset of the > former. Remove the redundant check to simplify the code and for clarity. That is highly configuration dependent. #else /* CONFIG_DEVICE_PRIVATE */ ... static inline bool is_device_private_entry(swp_entry_t entry) { return false; } ... #if defined(CONFIG_MEMORY_FAILURE) || defined(CONFIG_MIGRATION) || \ defined(CONFIG_DEVICE_PRIVATE) static inline int non_swap_entry(swp_entry_t entry) { return swp_type(entry) >= MAX_SWAPFILES; } #else static inline int non_swap_entry(swp_entry_t entry) { return 0; } #endif
On Fri, Jun 12, 2020 at 12:35:24PM -0700, Matthew Wilcox wrote: > On Fri, Jun 12, 2020 at 12:26:18PM -0700, Ralph Campbell wrote: > > In zap_pte_range(), the check for non_swap_entry() and > > is_device_private_entry() is redundant since the latter is a subset of the > > former. Remove the redundant check to simplify the code and for clarity. > > That is highly configuration dependent. > > #else /* CONFIG_DEVICE_PRIVATE */ > ... > static inline bool is_device_private_entry(swp_entry_t entry) > { > return false; > } The commit message might be a bit confusing, as it is not a subset, I would say that device_private_entry alone is sufficient to tell if the entry is private or not. For the !CONFIG_DEVICE_PRIVATE case having it wired to false is right. Jason
On 6/12/20 12:33 PM, Jason Gunthorpe wrote: > On Fri, Jun 12, 2020 at 12:26:18PM -0700, Ralph Campbell wrote: >> In zap_pte_range(), the check for non_swap_entry() and >> is_device_private_entry() is redundant since the latter is a subset of the >> former. Remove the redundant check to simplify the code and for clarity. >> >> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com> >> >> This is based on the current linux tree and is intended for Andrew's mm >> tree. There is no rush so it could go into 5.9 but I think it is safe >> enough to go into an rc after the patch is reviewed. > > Probably shouldn't mark it as mm/hmm if Andrew is going to pick it OK, I'll mark it as just mm: if I need to repost. > Reviewed-by: Jason Gunthorpe <jgg@mellanox.com> > > Jason > Thanks!
On 6/12/20 12:42 PM, Jason Gunthorpe wrote: > On Fri, Jun 12, 2020 at 12:35:24PM -0700, Matthew Wilcox wrote: >> On Fri, Jun 12, 2020 at 12:26:18PM -0700, Ralph Campbell wrote: >>> In zap_pte_range(), the check for non_swap_entry() and >>> is_device_private_entry() is redundant since the latter is a subset of the >>> former. Remove the redundant check to simplify the code and for clarity. >> >> That is highly configuration dependent. >> >> #else /* CONFIG_DEVICE_PRIVATE */ >> ... >> static inline bool is_device_private_entry(swp_entry_t entry) >> { >> return false; >> } > > The commit message might be a bit confusing, as it is not a subset, I > would say that device_private_entry alone is sufficient to tell if the > entry is private or not. > > For the !CONFIG_DEVICE_PRIVATE case having it wired to false is > right. > > Jason > How about the following message instead? In zap_pte_range(), the check for non_swap_entry() and is_device_private_entry() is unnecessary since the latter is sufficient to determine if the page is a device private page. Remove the test for non_swap_entry() to simplify the code and for clarity.
On Fri, Jun 12, 2020 at 12:53:23PM -0700, Ralph Campbell wrote: > > On 6/12/20 12:42 PM, Jason Gunthorpe wrote: > > On Fri, Jun 12, 2020 at 12:35:24PM -0700, Matthew Wilcox wrote: > > > On Fri, Jun 12, 2020 at 12:26:18PM -0700, Ralph Campbell wrote: > > > > In zap_pte_range(), the check for non_swap_entry() and > > > > is_device_private_entry() is redundant since the latter is a subset of the > > > > former. Remove the redundant check to simplify the code and for clarity. > > > > > > That is highly configuration dependent. > > > > > > #else /* CONFIG_DEVICE_PRIVATE */ > > > ... > > > static inline bool is_device_private_entry(swp_entry_t entry) > > > { > > > return false; > > > } > > > > The commit message might be a bit confusing, as it is not a subset, I > > would say that device_private_entry alone is sufficient to tell if the > > entry is private or not. > > > > For the !CONFIG_DEVICE_PRIVATE case having it wired to false is > > right. > > > > Jason > > > > How about the following message instead? > > In zap_pte_range(), the check for non_swap_entry() and > is_device_private_entry() is unnecessary since the latter is sufficient > to determine if the page is a device private page. Remove the test for > non_swap_entry() to simplify the code and for clarity. Yes, that is clearer to me Jason
diff --git a/mm/memory.c b/mm/memory.c index dc7f3543b1fd..bdbb4f97e7d0 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -1098,7 +1098,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, } entry = pte_to_swp_entry(ptent); - if (non_swap_entry(entry) && is_device_private_entry(entry)) { + if (is_device_private_entry(entry)) { struct page *page = device_private_entry_to_page(entry); if (unlikely(details && details->check_mapping)) {
In zap_pte_range(), the check for non_swap_entry() and is_device_private_entry() is redundant since the latter is a subset of the former. Remove the redundant check to simplify the code and for clarity. Signed-off-by: Ralph Campbell <rcampbell@nvidia.com> --- This is based on the current linux tree and is intended for Andrew's mm tree. There is no rush so it could go into 5.9 but I think it is safe enough to go into an rc after the patch is reviewed. mm/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)