mm_pages_next() question

mm_pages_next() question

[Avi Kivity]

> static int mmu_pages_next(struct kvm_mmu_pages *pvec,
>                           struct mmu_page_path *parents,
>                           int i)
> {
>         int n;
>
>         for (n = i+1; n < pvec->nr; n++) {
>                 struct kvm_mmu_page *sp = pvec->page[n].sp;
>
>                 if (sp->role.level == PT_PAGE_TABLE_LEVEL) {
>                         parents->idx[0] = pvec->page[n].idx;
>                         return n;
>                 }
>
>                 parents->parent[sp->role.level-2] = sp;
>                 parents->idx[sp->role.level-1] = pvec->page[n].idx;
>         }
>
>         return n;
> }

Do we need to break out of the loop if we switch parents during the loop 
(since that will give us a different mmu_page_path)?  Or are callers 
careful to only pass pvecs which belong to the same shadow page?

-- 
error compiling committee.c: too many arguments to function

Re: mm_pages_next() question

[Marcelo Tosatti]

On Sun, Mar 29, 2009 at 03:24:08PM +0300, Avi Kivity wrote:
>> static int mmu_pages_next(struct kvm_mmu_pages *pvec,
>>                           struct mmu_page_path *parents,
>>                           int i)
>> {
>>         int n;
>>
>>         for (n = i+1; n < pvec->nr; n++) {
>>                 struct kvm_mmu_page *sp = pvec->page[n].sp;
>>
>>                 if (sp->role.level == PT_PAGE_TABLE_LEVEL) {
>>                         parents->idx[0] = pvec->page[n].idx;
>>                         return n;
>>                 }
>>
>>                 parents->parent[sp->role.level-2] = sp;
>>                 parents->idx[sp->role.level-1] = pvec->page[n].idx;
>>         }
>>
>>         return n;
>> }
>
> Do we need to break out of the loop if we switch parents during the loop  
> (since that will give us a different mmu_page_path)?  Or are callers  
> careful to only pass pvecs which belong to the same shadow page?

This function builds mmu_page_path for a number of pagetable (leaf)
pages. Whenever the path changes, mmu_page_path will be rebuilt.

The pages in the pvec must be organized as follows:

level4, level3, level2, level1, level1, level1, ...., level3, level2,
level1, level1, ...

So you don't have to repeat higher levels for a number of leaf pages.

Re: mm_pages_next() question

[Avi Kivity]

Marcelo Tosatti wrote:
> On Sun, Mar 29, 2009 at 03:24:08PM +0300, Avi Kivity wrote:
>   
>>> static int mmu_pages_next(struct kvm_mmu_pages *pvec,
>>>                           struct mmu_page_path *parents,
>>>                           int i)
>>> {
>>>         int n;
>>>
>>>         for (n = i+1; n < pvec->nr; n++) {
>>>                 struct kvm_mmu_page *sp = pvec->page[n].sp;
>>>
>>>                 if (sp->role.level == PT_PAGE_TABLE_LEVEL) {
>>>                         parents->idx[0] = pvec->page[n].idx;
>>>                         return n;
>>>                 }
>>>
>>>                 parents->parent[sp->role.level-2] = sp;
>>>                 parents->idx[sp->role.level-1] = pvec->page[n].idx;
>>>         }
>>>
>>>         return n;
>>> }
>>>       
>> Do we need to break out of the loop if we switch parents during the loop  
>> (since that will give us a different mmu_page_path)?  Or are callers  
>> careful to only pass pvecs which belong to the same shadow page?
>>     
>
> This function builds mmu_page_path for a number of pagetable (leaf)
> pages. Whenever the path changes, mmu_page_path will be rebuilt.
>
> The pages in the pvec must be organized as follows:
>
> level4, level3, level2, level1, level1, level1, ...., level3, level2,
> level1, level1, ...
>
> So you don't have to repeat higher levels for a number of leaf pages.
>   

I'm still missing something.   That if () tests for level == 
PT_PAGE_TABLE_LEVEL.  So it looks like we'll have batch sizes of 4, 1, 
1, 1, ... 3, 1, 1, 1, ...?


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: mm_pages_next() question

[Marcelo Tosatti]

On Wed, Apr 01, 2009 at 11:40:30AM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> On Sun, Mar 29, 2009 at 03:24:08PM +0300, Avi Kivity wrote:
>>   
>>>> static int mmu_pages_next(struct kvm_mmu_pages *pvec,
>>>>                           struct mmu_page_path *parents,
>>>>                           int i)
>>>> {
>>>>         int n;
>>>>
>>>>         for (n = i+1; n < pvec->nr; n++) {
>>>>                 struct kvm_mmu_page *sp = pvec->page[n].sp;
>>>>
>>>>                 if (sp->role.level == PT_PAGE_TABLE_LEVEL) {
>>>>                         parents->idx[0] = pvec->page[n].idx;
>>>>                         return n;
>>>>                 }
>>>>
>>>>                 parents->parent[sp->role.level-2] = sp;
>>>>                 parents->idx[sp->role.level-1] = pvec->page[n].idx;
>>>>         }
>>>>
>>>>         return n;
>>>> }
>>>>       
>>> Do we need to break out of the loop if we switch parents during the 
>>> loop  (since that will give us a different mmu_page_path)?  Or are 
>>> callers  careful to only pass pvecs which belong to the same shadow 
>>> page?
>>>     
>>
>> This function builds mmu_page_path for a number of pagetable (leaf)
>> pages. Whenever the path changes, mmu_page_path will be rebuilt.
>>
>> The pages in the pvec must be organized as follows:
>>
>> level4, level3, level2, level1, level1, level1, ...., level3, level2,
>> level1, level1, ...
>>
>> So you don't have to repeat higher levels for a number of leaf pages.
>>   
>
> I'm still missing something.   That if () tests for level ==  
> PT_PAGE_TABLE_LEVEL.  So it looks like we'll have batch sizes of 4, 1,  
> 1, 1, ... 3, 1, 1, 1, ...?

The input is the pvec array, organized as follows: 4, 3, 2, 1, 1, 1,
2, 1, 1, 3, 2, 1.

The output will be:

1 (pvec position 4), 1 (pos 5), 1 (pos 6). All of them with the same
path. 

Then 1 (pos 8), 1 (pos 9). With the same path as before but level 2
being the page in position 7.

So the if() tests for level == PT_PAGE_TABLE_LEVEL because these are the
pages we're interested in walking (the ones that can be unsync). If
we're not PT_PAGE_TABLE_LEVEL, we walk the pvec building mmu_page_path.