On Wed Feb 18, 2026 at 6:00 PM JST, Eliot Courtney wrote:
<snip>
>>> +
>>> + self.send_single_command(bar, &wrapped)?;
>>> +
>>> + while let Some(continuation) = wrapped.next_continuation_record() {
>>> + dev_dbg!(
>>> + &self.dev,
>>> + "GSP RPC: send continuation: size=0x{:x}\n",
>>> + Self::command_size(&continuation),
>>> + );
>>> + self.send_continuation_record(bar, &continuation)?;
>>> + }
>>
>> Btw, can we recover if a split message fails between two continuation
>> records? I suspect the GSP will notice that the next message is not the
>> expected continuation record and recover from there?
>
> IIUC neither openrm or nouveau can recover from a failure during sending
> continuation records. What failure mode do you see happening that we
> could recover from?
None in particular, I just want to confirm we have thought about it. If
OpenRM considers this a catastrophic failure, doing the same in Nova is
fair enough.
<snip>
>>> +
>>> +/// The `ContinuationRecord` command.
>>> +pub(crate) struct ContinuationRecord<'a> {
>>> + data: &'a [u8],
>>> +}
>>> +
>>> +impl<'a> ContinuationRecord<'a> {
>>> + /// Creates a new `ContinuationRecord` command with the given data.
>>> + pub(crate) fn new(data: &'a [u8]) -> Self {
>>> + Self { data }
>>> + }
>>> +}
>>> +
>>> +impl<'a> CommandToGsp for ContinuationRecord<'a> {
>>> + const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
>>> + type Command = Empty;
>>> + type InitError = Infallible;
>>> +
>>> + fn init(&self) -> impl Init<Self::Command, Self::InitError> {
>>> + Empty::init_zeroed()
>>> + }
>>> +
>>> + fn variable_payload_len(&self) -> usize {
>>> + self.data.len()
>>> + }
>>> +
>>> + fn init_variable_payload(
>>> + &self,
>>> + dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
>>> + ) -> Result {
>>> + dst.write_all(self.data)
>>> + }
>>> +}
>>> +
>>> +/// Wrapper that splits a command across continuation records if needed.
>>> +pub(crate) struct WrappingCommand<C: CommandToGsp> {
>>> + inner: C,
>>> + offset: usize,
>>> + max_size: usize,
>>> + staging: KVVec<u8>,
>>
>> Since it is conditionally-used, `staging` should be an `Option` instead
>> of assuming an empty state means it is unused. But hold on, I think we
>> can do without any sort of conditional here.
>
> Yeah you are right. I had used empty here since it makes
> `next_continuation_record` here more consistent in that it doesn't need
> to check for None then for length anyway, but it's special casing in
> `init_variable_payload` in a very optiony way regardless.
>
> I reckon using Option we could put `offset` in there as well in a tuple,
> since if we want to use Option to avoid the special case of empty
> `staging`, we might as well use it to avoid having a conceptually
> useless `offset` field too.
All the more reason not to handle the non-continuation case in this
type. Let's keep it straightforward and responsible for only doing the
splitting of messages - if we indeed need another type to be able to
handle both cases, we can then compose one using an enum.
>
>>
>>> +}
>>
>> This deserves more doccomments, including on its members. But I would
>> also like to entertain a slightly different design.
>>
>> In this patch, `WrappingCommand` is always used, including for messages
>> that don't need to be truncated. While the overhead is arguably
>> negligible, this makes split messages pass as the norm, while they are a
>> very rare exception.
>>
>> Also `WrappingCommand` now becomes both a command and a provider of
>> other commands, which I find hard to wrap (haha) my head around, and
>> forces you to pass the command by reference in `send_single_command`
>> because you need to use it again afterwards. The name is also not very
>> descriptive (why does it wrap?). How about this instead:
>>
>> `send_command` is the central path that all commands take, so it is the
>> right place to check whether we need to use continuation records. If we
>> don't, then we just send the command as-is, as we do today.
>>
>> If we need to split, we do it through a private method of `Cmdq` that
>> consumes the command and returns a tuple `(SplitCommand<M>,
>> ContinuationRecords)`
>>
>> `SplitCommand<M>` can be a wrapper around the original command, but that
>> initializes the truncated part of its variable payload.
>>
>> `ContinuationRecords` implements `Iterator<Item = ContinuationRecord>`.
>> `ContinuationRecord` works mostly like your current version, except it
>> owns its data (yes, more allocations, but they're smaller so the
>> allocator might actually appreciate).
>
> Is it really the case that more, smaller allocations is better if we are
> using KVVec rather than KVec? It feels like it would have
> overhead/fragmentation to me. Eager to learn if this is wrong though
Ah, for some reason I read KVec here. KVVec is fine, and thinking about
it, we don't even need one allocation per continuation record: just one
for the main command, and one for the continuation records iterator
type, which the individual `ContinuationRecord`s can refer to using a
slice (basically, your current design).
>
>>
>> Since `SplitCommand` is only used when there is actually a split needed,
>> `staging` does not need an empty state anymore, and the code of each
>> type becomes simpler.
>>
>> By doing so, you also don't need to pass the command by reference in
>> `send_single_command` anymore.
>
> I considered putting the continuation record logic inside cmdq, but I
> feel it's nicer to keep it out of the core command queue logic as much
> as possible, because it feels a bit noisy to have in the cmdq which
> hopefully can just concentrate on sending messages, not how those
> messages get made or how/when they get split.
Fair enough. Are you suggesting that we use a new type instead of a
`Cmdq` function? If so, that looks good to me, it's basically turning
the tuple return value I suggested into its own type with a constructor.
That's arguably cleaner actually.
>
> I agree that it is a bit odd to have a command that also provides extra
> commands. By the way I considered doing an Iterator thing for
> WrappingCommand, but felt like it would introduce more complexity
> than it was worth and make it more confusing on the command that also
> produces commands front.
Ah, with the `ContinuationRecord`s borrowing from `self` it is difficult
to implement `Iterator` on `WrappingCommand` indeed. The current method
is fine, it's preferable to doing another copy and thankfully we don't
need the full power of iterators here.
>
> Another benefit to pulling the logic out of command queue is that it
> makes it a lot easier to test AFAICT. If we want to test the splitting,
> if the logic is a private method in the command queue that feels like it
> might end up more as an integration test.
>
> But if we just want to avoid having a command which also produces
> commands, what if we created a separate type to hold the split state /
> decision and it would return a SplitCommand plus either an iterator or
> the iterator-like `next_continuation_record`, while being testable
> separately. WDYT?
So basically, the simplified `SplitCommand` and its continuation
records, which we wrap into an enum holding the decision of whether the
command is split or not? That sounds fine.
I'd like to also keep the continuation records outside of
`SplitCommand`, as this will let us pass the commands by value to
`send_command`, which is a detail I like as it ensures we cannot send
the same command twice without explicitly cloning it before.
