[Kos-dev] Remarques sur le commit

[d2]

Bonjour,

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@enix.org> writes:
    Thomas> Ok, je comprends bien maintenant pourquoi on doit locker
    Thomas> les gpfme au niveau de pmm, j'avais pense aux flags et aux
    Thomas> listes, mais pas au ref_cnt effectivement.

Bon, maintenant que vous avez pige mon argumentation, que vous etes
convaincus et tout, a moi de m'amuser a la demonter : pas besoin de
lock du gpfme pour faire ca (le ref_cnt++/le ref_cnt==0). Il suffit
d'utiliser un "incl"/"cmpl" atomique (doit bien y avoir un prefixe
"lock" en IA32).

Ceci dit, ca me parait qd meme plus propre d'avoir la garantie
suivante, meme si je viens de dire qu'elle n'est *pour l'instant* pas
necessaire : "qd on est hors de pmm et qu'on s'interesse a un gpfme
(p.ex dans vmap/rmap), on a la garantie que tant qu'on le locke, le
gpfme ne va pas etre transfere vers une autre liste (ie on a la
garantie que ses flags ne vont pas changer en cours de route) suite a
l'action d'un autre thread (sur un autre processeur, s'entend)".

Bref, ces considerations de locking de gpfme ET de gpfm_lists dans
pmm, c'est plus par prevention que par necessite pour le moment. Je
pense que ce sera plus confortable d'avoir ces garanties quand on
sera dans le swapper.

    Thomas> J'avoue n'avoir pas bien compris le role de la garbage
    Thomas> liste. J'ai bien lu le commentaire "[GARBAGE Note] We
    Thomas> cannot free the gpfme because we cannot tell whether
    Thomas> another thread is still using it => invalidate it and move
    Thomas> it into the garbage list.", mais je ne vois pas bien le
    Thomas> truc.

Si je fais :

    thread 0                                 thread 1
    gpfme=get_gpfme_at_paddr(out of RAM)     ...
    <preemption>                         ->  put_physical_page(meme adresse)
    ...                                      (c'etait PAS une zone
                                             "allocatable")
                                                => kfree(gpfme associe)
    add_rmapping(gpfme, vaddr)           <-  <preemption>
      ### BANG ###

=> le gpfme n'est plus valide au moment ou on fait le add_rmapping,
puisqu'il a ete libere par kfree entre temps.

Bon, bref, la seule solution serait que le get_gpfme_at_paddr renvoie
le gpfme *locke*. Mais le probleme est que, avec l'ordre de locking
gpfme puis lists, puisqu'on est dans le cas {out of RAM non
allocatable, ie mapping type carte Video}, le code de
get_gpfme_at_paddr() se deroule jusqu'a parcourir une liste. Donc on
doit locker la liste d'abord, en consequence de quoi on ne peut pas
locker le gpfme pdt le parcours des listes (sinon deadlock). Donc
necessairement, dans ce cas on a une zone franche pdt laquelle ni les
listes ni les gpfme ne sont lockees, quoi qu'on fasse. Donc impossible
de renvoyer le gpfme locké de facon *sure* (ie pas de kfree possible
entre temps).

Vu la rarete de la chose (demapping d'un periph physique, on en fait
pas 1283612837 par uptime systeme), j'ai pris le parti de considerer
que cette petite fuite de 28B par page demappee n'etait pas
dramatique.

Ceci dit, rien que pour la proprete du geste, la solution de prendre
l'ordre de locking oppose a celui choisi se defend. Si ce n'etait que
j'aimerais tester avant le coup du "1 lock par liste de gpfm", qui
incite fortement a conserver l'ordre de locking actuel.

    Thomas> y a plusieurs types de pages : - les pages noyau NON
    Thomas> swappables POUR TOUJOURS - les pages noyau NON swappable
    Thomas> POUR le moment - les pages noyau swappable POUR le moment
    Thomas> - les pages user (a priori forcement swappable) - les
    Thomas> pages hw, non swappables (forcement)

C'est vrai, mon test etait faux et ne convenait pas. C'est ca :
      RETURN_VAL_IF_FAILED(gpfme->frags.page_type != PHYS_PAGE_KERNEL, -1)
que j'aurais du ecrire.

Car je pars du principe qu'une page noyau n'a pas le droit de changer
de status envers le swap. Si elle est swappable a 1 moment, elle l'est
pour tjs. De meme, les pages noyau non swappables a 1 moment le sont
pour tjs. Par contre, les pages user ont le droit de changer en cours
de route.

Thomas, tu proposes que /certaines/ pages noyau puissent avoir la
possibilite de changer de swap_status en cours de route. Honnetement,
je ne sais pas quelle approche est la plus correcte.

    Thomas> Je vois pas bien a quoi servent
    Thomas> new_additionnal_allocatable_page et
    Thomas> remove_additional_allocatable_page. Le nom indique que
    Thomas> c'est pour ajouter (supprimer) de la memoire allouable,
    Thomas> mais je vois pas pourquoi on pourrait ajouter comme ca de
    Thomas> la memoire allouable, et surtout a quoi ca
    Thomas> servirait. Surement un truc qui m'echappe, mais je ne vois
    Thomas> pas !

Dans la mesure ou ca ne rajoute pas de complexite dans le code, je me
suis dit que ca peut etre interessant de pouvoir rajouter de la RAM en
cours de fonctionnement. Style-genre, on a un peripherique vachement
evolué qui emule de la memoire physique, alors qu'en fait il s'agit
d'une carte reseau reseau speciale DSM. C'est donc plus du domaine du
fantasme qu'autre chose. Mais ca ne complique pas le code. C'est le
demapping de peripheriques out of RAM "non allocatable" qui complique
tout (avec la liste garbage).

Bonne journee,

-- 
d2