Repository navigation
fix(qwen36 tier): keep the per-device block inside its buffer, and wake every waiter on shutdown - #1348
fix(qwen36 tier): keep the per-device block inside its buffer, and wake every waiter on shutdown#1348JustVugg wants to merge 1 commit into
Conversation
…ke every waiter on shutdown Due difetti preesistenti trovati da crichalchemist mentre lavorava a #1338. Nessuno dei due viene da #1334: sono nel codice che quella PR ha solo portato all'attenzione. #1339 -- G.is_x era allocato 32*D in tutto, cioe' la capienza di UN device, mentre l'indirizzamento assumeva un blocco per ciascuno con passo 8*D. Con due o piu' GPU il device 1 scriveva da 8*D fino a 8*D+32*D, oltre la fine. Tre numeri scritti a mano in tre punti (32, 8, 32) e due non erano d'accordo. Ora esistono qt_is_offset e qt_is_floats e sono l'unica fonte: il passo e la capienza non possono piu' divergere perche' sono la stessa costante, e il buffer si dimensiona su ndev. #1340 -- th_stop lo guardano DUE condizioni, e qt_shutdown segnalava solo cv. Chi era fermo su cv_take -- coda piena in enqueue_locked, o un gruppo in volo nell'uploader -- non si svegliava, e il pthread_join dentro qt_shutdown restava appeso. Ora broadcast su entrambe: fermarsi non deve dipendere da quale delle due attese e' capitata. Controlli negativi, entrambi mordono: - rimesso il signal singolo, il test SI APPENDE (timeout) -- che e' esattamente il guasto reale, quindi il blocco e' la diagnosi - rimesso il passo 8*D, due asserzioni falliscono nominando il difetto La prima versione del test su #1339 NON mordeva: ricalcolava l'aritmetica con le costanti invece di chiamare il codice, quindi restava verde col passo sbagliato rimesso -- verificava se stessa. E' il motivo per cui l'indirizzamento e' stato estratto in funzione: cosi' il test chiama cio' che chiama qt_issue.
|
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. |
1 participant
Fixes #1339 and #1340, both reported by @crichalchemist while working on #1338. Neither comes from #1334 — I checked my diff: zero lines of mine in
qt_issue,cv_takeoris_x. They are pre-existing, surfaced by the attention that PR drew.#1339 — the per-device block ran past its buffer
G.is_xwas allocated32*Din total — one device's worth — while the addressing assumed a block each at stride8*D. With two or more GPUs, device 1 wrote from8*Dto8*D + 32*D. Three hand-written numbers in three places (32, 8, 32), two of which disagreed.qt_is_offset()andqt_is_floats()are now the single source: stride and capacity cannot drift apart because they are the same constant, and the buffer is sized byndev.#1340 — shutdown woke only half the waiters
th_stopis watched by two conditions andqt_shutdownsignalled onlycv. Anyone parked oncv_take— a full queue inenqueue_locked, or a group in flight in the uploader — never woke, and thepthread_joininsideqt_shutdownhung. Now both are broadcast: stopping must not depend on which of the two waits happened to be in progress.Negative controls, both bite
signalon shutdown8*DstrideMy first test for #1339 did not bite. It recomputed the arithmetic from the constants instead of calling the code, so it stayed green with the wrong stride restored — it verified itself. That is why the addressing was extracted into functions: so the test calls what
qt_issuecalls. The negative control is the only reason I found out.