Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1408841 > unrolled thread

Re: [RFC PATCH 2/4] mm: Change the interface for __tlb_remove_page

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 2/4] mm: Change the interface for __tlb_remove_page "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-05-30 10:10 +0200
    Re: [RFC PATCH 2/4] mm: Change the interface for __tlb_remove_page "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2016-05-30 17:40 +0200
      Re: [RFC PATCH 2/4] mm: Change the interface for __tlb_remove_page "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-05-31 06:00 +0200
        Re: [RFC PATCH 2/4] mm: Change the interface for __tlb_remove_page "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2016-05-31 09:00 +0200
          Re: [RFC PATCH 2/4] mm: Change the interface for __tlb_remove_page "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-05-31 09:40 +0200

#1408841 — Re: [RFC PATCH 2/4] mm: Change the interface for __tlb_remove_page

> diff --git a/mm/memory.c b/mm/memory.c
> index 15322b73636b..a01db5bc756b 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -292,23 +292,24 @@ void tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long e
>   *	handling the additional races in SMP caused by other CPUs caching valid
>   *	mappings in their TLBs. Returns the number of free page slots left.
>   *	When out of page slots we must call tlb_flush_mmu().
> + *returns true if the caller should flush.
>   */
> -int __tlb_remove_page(struct mmu_gather *tlb, struct page *page)
> +bool __tlb_remove_page(struct mmu_gather *tlb, struct page *page)
>  {
>  	struct mmu_gather_batch *batch;
> 
>  	VM_BUG_ON(!tlb->end);
> 
>  	batch = tlb->active;
> -	batch->pages[batch->nr++] = page;
>  	if (batch->nr == batch->max) {
>  		if (!tlb_next_batch(tlb))
> -			return 0;
> +			return true;
>  		batch = tlb->active;
>  	}
>  	VM_BUG_ON_PAGE(batch->nr > batch->max, page);

Still needed?
> 
> -	return batch->max - batch->nr;
> +	batch->pages[batch->nr++] = page;
> +	return false;
>  }
> 
>  #endif /* HAVE_GENERIC_MMU_GATHER */
> @@ -1109,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>  	pte_t *start_pte;
>  	pte_t *pte;
>  	swp_entry_t entry;
> +	struct page *pending_page = NULL;
> 
>  again:
>  	init_rss_vec(rss);
> @@ -1160,8 +1162,9 @@ again:
>  			page_remove_rmap(page, false);
>  			if (unlikely(page_mapcount(page) < 0))
>  				print_bad_pte(vma, addr, ptent, page);
> -			if (unlikely(!__tlb_remove_page(tlb, page))) {
> +			if (unlikely(__tlb_remove_page(tlb, page))) {
>  				force_flush = 1;
> +				pending_page = page;
>  				addr += PAGE_SIZE;
>  				break;
>  			}
> @@ -1202,7 +1205,12 @@ again:
>  	if (force_flush) {
>  		force_flush = 0;
>  		tlb_flush_mmu_free(tlb);
> -
> +		if (pending_page) {
> +			/* remove the page with new size */
> +			__tlb_adjust_range(tlb, tlb->addr);

Would you please specify why tlb->addr is used here?

thanks
Hillf 
> +			__tlb_remove_page(tlb, pending_page);
> +			pending_page = NULL;
> +		}
>  		if (addr != end)
>  			goto again;
>  	}
> --
> 2.7.4

[toc] | [next] | [standalone]

#1409236

Hillf Danton <hillf.zj@alibaba-inc.com> writes:

>> diff --git a/mm/memory.c b/mm/memory.c
>> index 15322b73636b..a01db5bc756b 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -292,23 +292,24 @@ void tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long e
>>   *	handling the additional races in SMP caused by other CPUs caching valid
>>   *	mappings in their TLBs. Returns the number of free page slots left.
>>   *	When out of page slots we must call tlb_flush_mmu().
>> + *returns true if the caller should flush.
>>   */
>> -int __tlb_remove_page(struct mmu_gather *tlb, struct page *page)
>> +bool __tlb_remove_page(struct mmu_gather *tlb, struct page *page)
>>  {
>>  	struct mmu_gather_batch *batch;
>> 
>>  	VM_BUG_ON(!tlb->end);
>> 
>>  	batch = tlb->active;
>> -	batch->pages[batch->nr++] = page;
>>  	if (batch->nr == batch->max) {
>>  		if (!tlb_next_batch(tlb))
>> -			return 0;
>> +			return true;
>>  		batch = tlb->active;
>>  	}
>>  	VM_BUG_ON_PAGE(batch->nr > batch->max, page);
>
> Still needed?

yes, we need to make sure the batch we picked doesn't have a wrong
batch->nr value.

>> 
>> -	return batch->max - batch->nr;
>> +	batch->pages[batch->nr++] = page;
>> +	return false;
>>  }
>> 
>>  #endif /* HAVE_GENERIC_MMU_GATHER */
>> @@ -1109,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>>  	pte_t *start_pte;
>>  	pte_t *pte;
>>  	swp_entry_t entry;
>> +	struct page *pending_page = NULL;
>> 
>>  again:
>>  	init_rss_vec(rss);
>> @@ -1160,8 +1162,9 @@ again:
>>  			page_remove_rmap(page, false);
>>  			if (unlikely(page_mapcount(page) < 0))
>>  				print_bad_pte(vma, addr, ptent, page);
>> -			if (unlikely(!__tlb_remove_page(tlb, page))) {
>> +			if (unlikely(__tlb_remove_page(tlb, page))) {
>>  				force_flush = 1;
>> +				pending_page = page;
>>  				addr += PAGE_SIZE;
>>  				break;
>>  			}
>> @@ -1202,7 +1205,12 @@ again:
>>  	if (force_flush) {
>>  		force_flush = 0;
>>  		tlb_flush_mmu_free(tlb);
>> -
>> +		if (pending_page) {
>> +			/* remove the page with new size */
>> +			__tlb_adjust_range(tlb, tlb->addr);
>
> Would you please specify why tlb->addr is used here?
>

That is needed because tlb_flush_mmu_tlbonly() does a __tlb_reset_range().


>> +			__tlb_remove_page(tlb, pending_page);
>> +			pending_page = NULL;
>> +		}
>>  		if (addr != end)
>>  			goto again;
>>  	}
>> --
>> 2.7.4

-aneesh

[toc] | [prev] | [next] | [standalone]

#1409795

> >> @@ -1202,7 +1205,12 @@ again:
> >>  	if (force_flush) {
> >>  		force_flush = 0;
> >>  		tlb_flush_mmu_free(tlb);
> >> -
> >> +		if (pending_page) {
> >> +			/* remove the page with new size */
> >> +			__tlb_adjust_range(tlb, tlb->addr);
> >
> > Would you please specify why tlb->addr is used here?
> >
> 
> That is needed because tlb_flush_mmu_tlbonly() does a __tlb_reset_range().
> 
If ->addr is updated in resetting, then it is a noop here to deliver tlb->addr to
__tlb_adjust_range().
On the other hand, if ->addr is not updated in resetting, then it is also a noop here.

Do you want to update ->addr here?

thanks
Hillf

[toc] | [prev] | [next] | [standalone]

#1409909

Hillf Danton <hillf.zj@alibaba-inc.com> writes:

>> >> @@ -1202,7 +1205,12 @@ again:
>> >>  	if (force_flush) {
>> >>  		force_flush = 0;
>> >>  		tlb_flush_mmu_free(tlb);
>> >> -
>> >> +		if (pending_page) {
>> >> +			/* remove the page with new size */
>> >> +			__tlb_adjust_range(tlb, tlb->addr);
>> >
>> > Would you please specify why tlb->addr is used here?
>> >
>> 
>> That is needed because tlb_flush_mmu_tlbonly() does a __tlb_reset_range().
>> 
> If ->addr is updated in resetting, then it is a noop here to deliver tlb->addr to
> __tlb_adjust_range().
> On the other hand, if ->addr is not updated in resetting, then it is also a noop here.
>
> Do you want to update ->addr here?
>

I don't get that question. We wanted to track the alst adjusted addr in
tlb->addr because when we do a tlb_flush_mmu_tlbonly() we does a
__tlb_reset_range(), which clears tlb->start and tlb->end. Now we need
to update the range again with the last adjusted addr before we can call
__tlb_remove_page(). Look for VM_BUG_ON(!tlb->end); in
__tlb_remove_page().

-aneesh

[toc] | [prev] | [next] | [standalone]

#1409934

> > Do you want to update ->addr here?
> >
> 
> I don't get that question. We wanted to track the alst adjusted addr in
> tlb->addr because when we do a tlb_flush_mmu_tlbonly() we does a
> __tlb_reset_range(), which clears tlb->start and tlb->end. Now we need
> to update the range again with the last adjusted addr before we can call
> __tlb_remove_page(). Look for VM_BUG_ON(!tlb->end); in
> __tlb_remove_page().
> 
Got, thanks.

Hillf

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web