[Kos-dev] Remarques sur le commit

[d2]

Salut,

Premiere (partie de) question (de Thomas) : pourquoi faut-il locker
les gpfme ET les listes de gpfm dans pmm ? La reponse est que quand on
possede gpfme->lock, on veut etre ASSURE que les flags dans le gpfme
ne changeront pas. En d'autres termes, on veut etre assure que quand
on locke un gpfme, personne d'autre que nous ne peut y toucher, meme
pas pmm !

"Et pourquoi ?"... Bonne question. En effet, en apparence, qd je suis
dans vmap (plus precisement : dans rmap), les flags du gpfme, je m'en
sers pas (je ne les regarde jamais), donc on pourrait penser qu'il est
pas necessaire d'interdire leur modification par pmm qd je possede le
gpfme->lock. Erreur : rmap touche a gpfme->ref_cnt, OR put_phys_page()
regarde ce gpfme->ref_cnt avant de liberer une page. Et donc on doit
interdire toute action de rmap quand on est dans put_phys_page() entre
le moment ou on regarde le gpfme->ref_cnt, et le moment ou le gpfme va
etre transfere dans la liste des free ! Il faut donc locker ce gpfme
pdt toute la periode de transfert d'une liste a l'autre.

Donc, meme au niveau peu avance ou nous sommes (pas encore de
swapper), si on definit un lock par gpfme, il FAUT que pmm locke a la
fois le gpfme ET les listes au moins a 1 endroit (ie dans
put_phys_page()). Pour eviter les race conditions internes a pmm, si
on fait ca a 1 endroit, on est donc oblige de generaliser ca partout
damns pmm : partout ou on touche a 1 gpfme particulier pour le bouger,
on doit le locker (et locker les listes aussi).

La 2eme question qui se pose, c'est l'ordre dans lequel locker
gpfm_lists et gpfme, qd on doit locker les 2 (ie bcp d'endroits dans
pmm). En effet, cet ordre de locking doit rester le meme partout
(sinon deadlocks), et l'inverse pour unlocking. Evidemment, seul pmm
est concerne ; en dehors de pmm, seul gpfme->lock est a
considerer. Alors la 2eme question est : "dans pmm, donc, ordre {gpfme
puis gpfm_lists}, ou ordre {gpfm_lists puis gpfme} ?"  J'ai choisi
d'implanter l'ordre de locking gpfme->lock puis gpfm_lists pour locker
(et l'inverse pour unlocker).

Les 2 approches sont possibles. Le seul argument que j'ai est
developpe plus loin (possibilite d'avoir 1 lock par liste).

Au debut, j'etais parti pour l'ordre inverse : gpfm_lists PUIS
gpfme->lock. Puis finalement j'ai chosi l'ordre indiqué. C'est parce
qu'au debut, je voulais 1 lock par liste gpfm (voir plus loin). Donc
j'ai change l'ordre de locking. La-dessus sont arrivees mes reflexions
menant a la liste garbage, qui ont fait disparaitre ce 1 lock par
liste a une etape donnee de ces reflexions. Apres reflexions, on
pourrait re-implanter ce "1 lock par liste" (voir plus loin).

Bon, mais passons. A noter le probleme interessant suivant dans
l'implantation actuelle de get_physical_page(). Ce probleme est une
intro aux difficultes que causeraient la cohabitation "ordre {lock
listes gpfm, puis gpfme->lock} ET 1 lock par liste gpfm". Ce probleme
precis est bel et bien lie a l'ordre choisi dans l'implantation
actuelle, en depit de la phrase precedente : gpfme->lock puis
gpfm_lists. Pour suivre : papier/crayon ;)

En effet, dans get_physical_page(), on doit d'abord locker la liste
des free (-> lock gpfm_lists), pour "voir sa tete". Or, puisqu'on ne
peut pas locker le gpfme en tete dans la mesure ou on a deja le lock
gpfm_lists, on est oblige de relacher le lock gpfm_lists. Maintenant
seulement, on peut prendre gpfme->lock. Mais entre temps, gpfme peut
avoir ete change de liste par un autre thread. Donc il faut verifier a
ce niveau que le gpfme est tjs free. Si oui, on peut locker les listes
et faire le transfert vers la liste destination. Sinon, faut
recommencer a regarder la nouvelle tete de la liste free (normalement
differente [sauf cas extremes, mais qui ne posent pas de pb], vu qu'un
autre thread a reussi a changer le type de l'ancienne tete a autre
chose que FREE, c'est donc aussi qu'il a reussi a transferer
l'ancienne tete vers une autre liste). D'ou le do { ... } while(TRUE);
(avec un break au milieu).

On remarque qu'on a donc une petite chose un peu delicate a ce
niveau. Mais dans l'ensemble, en conservant l'ordre de locking actuel,
ca ne compliquerait pas grd chose d'avoir 1 lock par liste de gpfm.

On remarque aussi qu'en faisant dans l'ordre inverse (gpfm_lists puis
gpfme->lock), on n'aurait plus la petite manip au niveau du
get_physical_page(). Mais dans ce cas, c'est moins raisonnable
d'envisager 1 lock par liste, comme on peut y penser actuellement. En
effet, dans pmm on passe notre temps a transferer les gpfme d'une
liste a l'autre : il faut donc savoir a quelle liste appartient le
gpfme avant de le bouger. Dans le cas ou on a 1 lock par liste avec
l'ordre de lock gpfm_lists puis gpfme->lock, ca necessite de faire la
meme manip que la manip actuelle du get_physical_page, mais dans leS
autreS fonctionS (au pluriel ;). En effet, pour bouger le gpfme de
liste, on doit recuperer le gpfme et regarder a quelle liste il
appartient (-> lock du gpfme->lock). Pas question de faire le lock des
listes a ce niveau puisque ca irait a l'encontre de l'ordre de locking
choisi. Donc on delocke le gpfme->lock avant de locker la liste source
et la liste destination. On peut donc locker maintenant le gpfme->lock
et initier le transfert. Seulement il faut s'assurer, AVANT de
transferer, que le gpfme n'a pas ete bouge entre temps par un autre
thread (entre le unlock du gpfme->lock et le lock des listes). Si il
est tjs dans la meme liste (meme flags), tout va bien, le transfert
peut se faire. Sinon, faut refaire la manip : unlock du gpfme puis des
listes, puis lock du gpfme, recup liste source, delock gpfme, relock
des listes, relock gpfme, verif bonne liste...

Bref, si on choisit l'ordre de lock gpfm_lists puis gpfme->lock, on
peut faire une croix sur "1 lock par liste", parce que ca impliquerait
des do { ... } while(TRUE) analogues a celui de get_phys_page()
actuellement, mais a bcp plus d'endroits dans le code : partout ou il
y a des transferts d'un gpfme connu d'une liste a l'autre.

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@enix.org> writes:
    Thomas> - en bas de pmm.h, tu mets dans le commentaires (formate
    Thomas> pour doxygen je suppose), que swap_status peut etre soit
    Thomas> PHYS_PAGE_KERNEL_LOCKED, soit PHYS_PAGE_SWAPPABLE, soit
    Thomas> PHYS_PAGE_USER_LOCKED. Je suppose que c'est une erreur, et
    Thomas> que swap_status ne peut etre que PHYS_PAGE_NON_SWAPPABLE,
    Thomas> PHYS_PAGE_SWAPPABLE. Bref il serait peut etre mieux que
    Thomas> l'argument swap_status soit un booleen.

Oui. A rectifier.

    Thomas> - le change_gpfme_swap_status me gene. effectivement on
    Thomas> veut pouvoir locker/delocker une page physique (mlock,
    Thomas> munlock), mais il faudrait aussi avoir un mecanisme pour
    Thomas> proteger les pages du noyau. Certains seront swappables
    Thomas> (et donc on pourra changer l'etat de swappabilisation
    Thomas> entre swappable et non swappables) et d'autres seront non
    Thomas> swappables a tout jamais. Avec cette fonction j'ai
    Thomas> l'impression que meme si on veut que la page ne soit
    Thomas> jamais swappable, y'a toujours moyen de la rendre
    Thomas> swappable... (mais peut etre je me trompe).

Oui, c'est exact. Il suffirait de rajouter un :
  RETURN_VAL_IF_FAILED((gpfme->frags.page_type == PHYS_PAGE_KERNEL)
                       && (gpfme->frags.swap_status == PHYS_PAGE_NON_SWAPPABLE),
                       -1);
Au debut de la fonction.

    Thomas> gpfme. Le probleme c'est que la structure spinlock_t fait
    Thomas> soit 0 octets (DEADLOCK_CHECK desactive) ou 12 octets
    Thomas> environ (DEADLOCK_CHECK active), or le probleme c'est que
    Thomas> la taille des GPFME doit etre connue dans le loader (cf
    Thomas> commentaires dans les .h respectifs).
    Thomas> Je pensais donc a un
    Thomas> #ifdef DEADLOCK_CHECK #define GPFME_SIZE lataille+12 #else
    Thomas> #define GPFME_SIZE lataille #endif
    Thomas> Je vois pas vraiment d'autres solutions !

Oui, je vois bien le pb. Je vois une autre "solution" : virer l'alloc
du gpfm par le loader. Il ne sert a rien du tout dans le loader, non ?

    Thomas> Tu bosses sur quoi maintenant David (qu'on se marche pas
    Thomas> sur les pieds), je pensais commencais le locking des
    Thomas> GPFME, mais vu que tu bosses pas mal sur PMM en ce moment,
    Thomas> je vais bosser sur FAT/IDE, afin d'eviter les conflits.

Je bosse sur rien jusqu'a Vendredi ou Samedi (et c'est meme pas sur
que je fasse qqch ce WE). Je veux pas me reserver cette partie locking
des pmm. Si t'as des idees, des envies de tenter l'ordre gpfm_lists
puis gpfme->lock, etc... y'a pas de pb. Ce WE (ou le suivant), je
trouverai bien des trucs qui restent a faire ;) Dans mes projets
court terme (ce WE ou le suivant) : 1 lock par liste gpfm, arch_remap,
virer l'alloc gpfm du loader (?), create_kernel_rmap(), virer les
sections .init. Aucune exclusivite la-dessus entre-temps. Que ca se
sache ;) Et apres... babel (enfin) !

Bonne journee,

-- 
d2