Repository navigation
Conversation
#1341 di crichalchemist, ed e' una regressione mia di #1334. Il warmstart consegnava al tier i pesi int8 e la riga dopo li liberava: qt_note_planned(l, eidw, wg, wu, wd, ...); /* il tier RITIENE wg */ if (!keep8 && e->g) { free(e->g); ... } /* e qui lo si libera */ Su int4 quella free e' sicura, perche' cio' che si libera e' la copia int8 mentre il tier tiene e->g4, un'allocazione distinta, e slot_ensure_int8() sa ricostruire da li'. Su int8 il puntatore consegnato E' e->g: il tier resta con memoria morta per il re-staging, e slot_ensure_int8() -- che esce subito quando g4 e' NULL -- non ha nulla da cui ricostruire, quindi il fallback CPU dereferenzia NULL. La free ora vale solo per int4. Il costo e' onesto e va detto: un container int8 non ha una copia impacchettata a meta' dimensione da tenere al posto dell'altra, quindi il risparmio di RAM che l'int4 ottiene qui l'int8 non puo' ottenerlo. Fingere il contrario ha creato il bug. Il commento immediatamente sotto la riga che avevo aggiunto diceva gia' "su eviction si rimaterializza da g4" -- vero per int4, falso per int8. Il codice mi stava dicendo che la mia diramazione nuova rompeva la sua precondizione, e l'ho letto come descrizione di cio' che c'era intorno invece che come vincolo su cio' che stavo aggiungendo. Il test guadagna il contratto che mancava: il tier RITIENE il puntatore, quindi chi glielo passa non puo' liberarlo. Detto onestamente, quel controllo prova il lato tier e NON prende la violazione dal lato chiamante: rimettendo la free incondizionata, il test resta verde. Per prenderla servirebbe eseguire il warmstart con un tier CUDA vero, che qui non ho -- ed e' esattamente la categoria di difetto che un backend software renderebbe verificabile in CI.
…es it Risposta a 'risolve per sempre?': la prima versione diceva 'libera solo se int4', che funziona oggi e codifica la corrispondenza ATTUALE fra formato e proprieta' della memoria. Chi domani aggiunge un formato che aliasa e->g non ha motivo di sospettare che quel controllo parli di ownership: rifarebbe #1341 senza accorgersene. Ora la condizione e' 'wg != e->g' -- non liberare cio' che hai appena consegnato. Non nomina formati, quindi un formato futuro che aliasa e' corretto senza che nessuno se ne ricordi, e uno con copia separata continua a liberare come prima. Verificato su tutti i casi, compresi due formati che non esistono ancora: consegna-diversa libera, consegna-alias non libera, e->g nullo non libera.
Owner
Author
|
Closing in favour of #1344, which was opened 16 hours before this and covers all three issues. I did not check the PR list before starting — I read the issue, replied "send your fix", and then wrote my own anyway. That is duplicated effort I caused, and the apology is owed to @crichalchemist rather than to the queue. #1344 is also better where it counts most. I wrote in #1347 that the caller-side violation could not be caught without a GPU, and #1344's One idea from here is worth carrying over, and I have left it as a review comment on #1344 rather than a competing PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1341, reported by @crichalchemist. This is my regression from #1334.
The chain
On int4 that free is safe: what is freed is the int8 copy, while the tier holds
e->g4, a separate allocation, andslot_ensure_int8()can rebuild from it. On int8 the pointer handed over ise->g— dangling pointer for re-staging, andslot_ensure_int8()returns immediately wheng4is NULL, so the CPU fallback dereferences NULL.The comment directly under my change already said "on eviction slot_ensure_int8() rematerializes from g4" — true for int4, false for int8. The code was stating the precondition my new branch broke, and I read it as describing the surrounding code rather than constraining what I was adding.
Fixed so it stays fixed
My first version said
expert_is_int4. That works today and encodes the current correspondence between format and memory ownership — whoever adds a third format that aliasese->ghas no reason to suspect that check is about ownership.The condition now names no format:
Do not free what you just handed over. A future aliasing format is correct without anyone remembering; a future format with a separate copy still frees as before. Verified across five cases including two formats that do not exist yet.
The honest cost
An int8 container has no half-size packed copy to keep instead of the full one, so the RAM saving int4 gets here is not available to int8. Pretending otherwise is what created the bug.
What the test does and does not do
The tier test gains the missing contract: the tier retains the pointer, so the caller must not free it.
Stated plainly: that check does not catch the caller-side violation. Restore the unconditional free and the test stays green, because the defect lives in the warmstart, which needs a real CUDA tier to execute. This is precisely the class of bug a software backend would make testable in CI — a concrete example rather than an argument.