[Lguest] [PATCH 1/1] Least Recently Used pgdirs

[Jason Yeh]

Glauber de Oliveira Costa wrote:
>>
>> The patch is made against Linus git tree. If there's a need to make the
>> patch agains the lguest mercurial server, please let me know.
>>
>>
> First of all, your mailer seems to be wrapping lines, and changing
> tabs to spaces. Try to avoid it.

I would like to thank both you and Rusty's comments and apology for the 
inconveniences caused by my email client. I will use external editor to 
send out patches in the future. As you can probably tell, I am rather 
new on sending out patches.

> It would be really interesting to see measures on that. From my POV,
> any complexity added to lguest must really pay off. Otherwise, we keep
> it simple.

I will do some benchmarking and see if the added complexity has any 
benefits after making suggested changes.

>> @@ -357,7 +361,7 @@ static void flush_user_mappings(struct lguest *lg,
>> int idx)
>>     unsigned int i;
>>     /* Release every pgd entry up to the kernel's address. */
>>     for (i = 0; i < vaddr_to_pgd_index(lg->page_offset); i++)
>> -        release_pgd(lg, lg->pgdirs[idx].pgdir + i);
>> +        release_pgd(lg, lg->pgdir_cur->pgdir + i);   }
> 
> It does not seem right. flush_user_mappings() not always release the
> mappings for the current pgdir.

As far as I can see, the only case flush_user_mappings() not releasing 
the current pgdir is in new_pgdir(), but the index is assigned to the 
current one after it returns. I will make it flexible for future 
purposes though.

>> /* We keep several page tables.  This is a simple routine to find the page
>>  * table (if any) corresponding to this top-level address the Guest has
>> given
>>  * us. */
>> -static unsigned int find_pgdir(struct lguest *lg, unsigned long pgtable)
>> +static struct pgdir * find_pgdir_from_list(struct lguest *lg, unsigned
>> long pgtable)
>> {
>> -    unsigned int i;
>> -    for (i = 0; i < ARRAY_SIZE(lg->pgdirs); i++)
>> -        if (lg->pgdirs[i].cr3 == pgtable)
>> +    struct pgdir *p, *rpgdir = NULL;
>> +   +    list_for_each_entry(p, &lg->pgdir_list, list) {
>> +        if (p->cr3 == pgtable) {
>> +            rpgdir  = p;
>> +            list_move(&rpgdir->list, &lg->pgdir_list);
> I am not sure about lguest32, but as for lguest64, there are
> situations in which finding a pgdir does not mean we're going to use
> it. So it would maybe be better to have a separate function to move
> things, once it is found. Brian Boitano (aka rusty) is the
> authoritative person for this.

I think that's the case in lguest32. I will dig into the lguest64 
patches and see what's the difference. I will do what Rusty suggested 
and see if moving makes any differences on performance.


>> +    /* Check if we reach the maximum number of pgdirs we can hold.
>> +     * Reuse the Least Recently Used one if we have. */
>> +    if (lg->pgdir_list_size > PGDIR_MAX)  {
>> +        newpgdir= list_entry(lg->pgdir_list.prev, struct pgdir, list);
>> +        list_move(&newpgdir->list, &lg->pgdir_list);
>> +    } else {
>> +        /* If it's never been allocated at all before, try now. */
>> +        newpgdir = kmalloc(sizeof(struct pgdir), GFP_KERNEL);
> kzalloc is probably better.
> 
>> +    newpgdir = kmalloc(sizeof(struct pgdir), GFP_KERNEL);
> again, kzalloc is probably better here.
> 
>> +    if (!newpgdir) {
>>         return -ENOMEM;
>> +    }
>> +
>> +    lg->pgdir_cur = newpgdir;
>> +    newpgdir->cr3 = pgtable;
>> +    newpgdir->pgdir = (spgd_t*)get_zeroed_page(GFP_KERNEL);
>> +    if (!newpgdir->pgdir)
>> +        return -ENOMEM;
> And what about your earlier allocation of newpgdir? Will it be here
> forever? free it before returning.

Good catch! I missed it.

Jason