[Kos-dev] DSR du commit de Thomas

[Thomas Petazzoni]

> Thomas vient de faire un commit pour rajouter les "DSR". Dans ce
> commit, un "DSR" est un handler analogue au handler d'IRQ normal, sauf
> qu'il est execute alors que les interruptions sont activees ("sti"
> avant).
> Le code me parait correct. Mais c'est pas tout a fait comme ca que
> j'imaginais les choses. Comme c'est qqch qui peut cependant s'averer
> utile, c'est a garder a mon avis. Je dirais meme que ce qui suit peut
> pleinement tirer parti des "DSR" du commit en question.

effectivement, apres avoir code le machin, je me suis demande quel etait
le cote Deferred dans tout ca. il n'y en avait effectivement pas !

> Pour que ce soit plus clair, je pense que le mieux est de renommer
> tout ca :
>   - ISR normal -> "clISR" ("cli" + "ISR")
>   - le "DSR" du commit en question  -> "stISR" ("sti" + "ISR")
>   - "vrai" DSR (equivalent BH ou Wait Queue Linux) -> "DSR"

ca me parait bien trop complexe d'avoir clISR, stISR, DSR puis proxy. je
pense que l'etape stISR que je viens "d'inventer" n'est pas vraiment
(voire pas du tout) utile. On peut surement placer les traitements a
faire la dedans soit dans le clISR, ou mieux dans le DSR. dans tous les
cas, 3 niveaux ca me paraissait deja bien complexe, mais 4 ca me semble
carrement disproportionne. qu'en pensez-vous ?

> Techniquement, les dsr_handler sont geres sous forme de files a
> priorite : on aurait DSR_LEVELS files (par exemple 32 ou 64 ou 4 ou 2
> ou 1 ou...). Ca veut dire que j'ai DSR_LEVELS listes (ou tableaux ; a
> voir) d'elements du type :

a quoi serviraient tous ces niveaux de DSR_LEVELS, comment tu les
attribue ?

>   - int add_dsr(int dsr_level,
>                 void (*routine)(int level, void *data), void
>                 *data));
>     => (0 si Ok, -1 si errreur)
>        Se charge de mettre a jour le bitmap + insere le nouveau dsr en
>        queue de la liste dsr_queue.level[dsr_level].

tout a fait d'accord. l'appel a cette fonction se fait par le clISR (ou
le stISR si il existe tjs).
 
>   - struct dsr_s * pop_dsr(int dsr_level);
>     => Se charge de mettre a jour le bitmap + enleve le dsr en tete de
>        la liste dsr_queue.level[dsr_level].

sera appele par le DSR lui meme quand il aura fini son execution.

> ------------------------------------
>   isr:
>        hw_isr_nested_level ++
>        save_context
> 
>        call clISR /* "ISR normal" */
> 
>        send_EOI
>        push hw_isr_nested_level /* TRES important */
> 
>        sti
>        call stISR /* "ton" DSR */
> 
>        if (pop eax == 1) /* C'etait l'irq la plus externe */ {
>          /* ^
>           * \--- Ce pop eax a pour but de recuperer le push
>           * hw_isr_nested_level fait plus haut. Il ne faut pas
>           * utiliser directement hw_isr_nested_level
>           */
>          call spawn_dsr /* Execution des DSR */
>          /*
>           * *TRES IMPORTANT* Retourne avec les interruptions
>           * desactivees : "cli" implicite
>           */
>        } else
>          cli /* TRES important */
> 
>        restore_context /* eventuellement nouveau contexte */
>        hw_isr_nested_level --
>        iret
> ------------------------------------

cote interruption je suis parfaitement d'accord avec ton schema, avec
toujours l'objection concernant l'utilite de l'existence du stISR.

> ------------------------------------
> /* *TRES IMPORTANT* Retourne avec les interruptions desactivees :
>    "cli" implicite */
> spawn_dsr() {
>   cli();
>   while (1) {
>     struct dsr_s *dsr;
>     int toplevel = bsf_bitmap(& dsr_queue.bitmap, sizeof(dsr_queue.bitmap));
>     if (toplevel >= DSR_LEVELS)
>       break; /* Plus de DSR en attente */
>     dsr = pop_dsr(toplevel);
>     if (!dsr || !dsr->routine)
>       continue;
>     sti();
>     dsr->routine(toplevel, dsr->data);
>     cli();
>   }
> }
> ------------------------------------

la encore, tout ceci me convient tout a fait. apres avoir vu tout ca je
comprends mieux ou nous voulions en venir avec nos bottom halves (enfin
DSR pardon). c'est deja nettement plus clair dans mon cerveau :)

> L'interet de tout ca ? Ces DSR permettent de serialiser certains
> traitements, ce qui evite d'avoir a prendre des precautions pour la
> protection de certaines donnees globales accedees uniquement dans les
> DSR. Ca complete la version DSR de Thomas, en la rendant plus fidele
> au sens de "Deferred". Le but de la manip est donc de retomber sur ses
> pied quant au vrai sens de "deferred".

ma version de DSR, appelle stISR est-elle vraiment utile ?

> Remarque 0 : ce mecanisme est extensible en SMP (tablequ de dsr_queue)..

perfect :)

> Remarque 1 : avec ce mecanisme, on peut par exemple mettre reschedule
> sous forme d'un DSR de plus faible priorite. Ainsi, le prochain thread
> a executer serait toujours determine en tout dernier lieu, a la suite
> de la toute derniere interruption la plus externe. Le probleme, c'est
> alors de savoir comment recuperer le contexte a executer depuis un
> DSR. Le mieux est sans doute d'avoir une variable globale qui
> s'appelle "next_cpu_context" (tableau en SMP), qui est positionnee par
> le reschedule, et qui constitue le prochain contexte a mettre avant le
> "iret".
> Remarque 2 : Si on choisit de faire comme la remarque 1 le suggere, on
> se heurte a un autre probleme de taille : dans ce cas, entre le
> reschedule et le iret, il peut tres bien y avoir un autre DSR qui
> provoque un #DF de pile !!!! Et donc notre hypothese forte sur "pas de
> #DF entre la fin du reschedule et le iret" n'est plus verifiee =>
> probleme.

a mon avis l'idee de mettre reschedule dans un DSR est plutot mauvaise.
j'avais deja pense a ca, et effectivement ce n'est pas du tout
compatible avec le double fault.
je me demande si ce double fault ne va finalement pas plus nous embeter
que nous arranger. toutefois est-ce dramatique de placer le reschedule()
dans le clISR comme cela est fait actuellement ?

thomas

PS : julien, tu es tjs vivant ?
-- 
PETAZZONI Thomas
thomas.petazzoni@meridon.com     UIN : 34937744
Projet KOS : http://kos.enix.org
Page Perso : http://www.enix.org/~thomas/