On Fri, Aug 05, 2005 at 11:37:33 +0000, Vincenzo Mallozzi wrote:
> On Thu 5 Aug 2005, at 07:02, Jan Hudec wrote:
> > Perhaps your code is safe, but I would think you should have the list
> > modification itself (list_add_tail) protected by a spin_lock_irqsave.
> > That's the only way to exclude against interrupts (you can't lock
> > semaphores in interrupt context).
> >
>
> But I'm developing a project in a uniprocessor environment, so I think it's
> unnecessary to lock code with spinlock, or, better, it's redundant.
That's not true since the preempt patch went in. Preempt is equivalent
to SMP.
> Now, I've a problem only with the use of INIT_LIST_HEAD.
> I've modified the code this way.
>
> 1. void mtpmc_set_mm_not_writable(struct mm_struct *mm)
> 2. {
> 3. struct vm_area_struct *vm;
> 4. struct mtpmc_wrprotected_pages *wr_page, *temp_page_wr;
> 5. struct mtpmc_vm_wrprotected *temp_vma_wr;
> 6. struct page *page;
> 7. pte_t *pte;
> 8. unsigned long addr;
> 9. int initial_page;
> 10.
> 11. down_write(&mm->mmap_sem);
> 12. for (vm = mm->mmap; vm!=NULL; vm=vm->vm_next)
> 13. if(mtpmc_vm_to_save(mm, vm) == 1){
> 14. temp_vma_wr = mtpmc_mk_vm_wrprotected(vm);
> 15. list_add_tail(&temp_vma_wr->list, &vm_write_protected);
> 16. initial_page = 1;
> 17. for(addr=vm->vm_start;addr<vm->vm_end;addr+=PAGE_SIZE){
> 18. pte = mtpmc_get_pte_from_address(mm, addr);
> 19. if (pte)
> 20. if (pte_write(*pte)){
> 21. set_pte(pte, pte_wrprotect(*pte));
> 22. if (initial_page == 1){
> 23. temp_vma_wr->pages = mtpmc_mk_page_wrprotected(addr);
> 24. INIT_LIST_HEAD(&(temp_vma_wr->pages)->list);
> 25. list_add_tail(&(temp_vma_wr->pages)->list,
> &(temp_vma_wr->pages)->list);
> 26. initial_page = 0;
> 27. }
> 28. else
> 29. list_add_tail(&mtpmc_mk_page_wrprotected(addr)->list,
> &(temp_vma_wr->pages)->list);
> 30. }
> 31. }
> 32. }
> 33. up_write(&mm->mmap_sem);
> 34.
> 35. return;
> 36. }
>
> The problem I encountered is in initialization of pages' list.
> Line regarding this problem are the ones from 22. to 30.
> This lines are executed after having saved the informations of the vma that
> I'm going to scan in the vm_write_protected list.
> After this, I've to store information regarding the infos of pages included in
> this vma. To do this, I've to create a new list containing the pages
> information.
> So, in line 22, I check (initial ==1) if the list for the pages of this vma
> is not just created. If so, I create it (line 24) and add to it the first
> element of this list (line 25).
> if test is false (initial == 0), then I simply add the new page to the list.
That sounds wrong to me. You should always INIT_LIST_HEAD when you
allocate the structure it is in and then you should simply add the
elements.
Oh, I don't see your structure definitions here, but the rule of thumb
is, that you *NEVER* ever have pointers to list_head in structures. You
should have list_head members.
That is, your temp_vma_wr->pages->list is supposed to be of type 'struct
list_head' -- NOT pointer -- and the call to INIT_LIST_HEAD is supposed
to be INIT_LIST_HEAD(&(temp_vma_wr->pages->list)) (or maybe pages.list,
but the & must apply to the whole argument in any case).
Say you have:
struct something {
list_head list;
list_head children;
some values;
}
struct otherthing {
list_head list;
other values;
}
DECLARE_LIST_HEAD(somethings);
And then you do:
struct something *alloc_something(...)
{
struct something *it = kmem_cache_alloc(something_cache,
GFP_KERNEL);
/* First let's initialize the list of children */
INIT_LIST_HEAD(&(it->children)); /* NOTE THE PARENTHESIS */
/* Other initialization */
/* And last, put it to the list */
list_add(&(it->list), &somethings);
return it;
}
struct otherthing *alloc_otherthing(struct something *st, ...)
{
struct otherthing *it = kmem_cache_alloc(otherthing_cache,
GFP_KERNEL);
/* Some initialization */
/* And last, put it to the list */
list_add(&(it->list), &(st->children));
return it;
}
Note, that the parenthesis after & are superfluous in all the cases.
I could have written INIT_LIST_HEAD(&it->children),
list_add(&it->list, &somethings) etc, but I have put them in for
clarity.
> Now, the problem is that, when I scans the list, the first element of the
> pages' lists is not present. In other word, the insertion of the first
> element is not correct.
> Can you help me on finding the problem? There's another way to organize this
> dynamic list?
> Thanks.
> Vincenzo Mallozzi
>
>
>
>
>
> ___________________________________
> Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB
> http://mail.yahoo.it
>
-------------------------------------------------------------------------------
Jan 'Bulb' Hudec <bulb@xxxxxx>
Attachment:
signature.asc
Description: Digital signature
