Re: [PATCH v2 7/8] gpu: nova-core: add NVKV typed decoding

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Sep 17, 2026 at 8:53 AM BST, Gary Guo wrote:
> On Mon Sep 14, 2026 at 7:55 AM BST, Eliot Courtney wrote:
>> On Mon Sep 14, 2026 at 12:46 PM JST, Alexandre Courbot wrote:
>>> On Thu Aug 27, 2026 at 11:12 PM JST, Eliot Courtney wrote:
>>>>  use crate::gsp::nvkv::{
>>>> +    Array,
>>>>      Index,
>>>> +    Key,
>>>>      KeyId,
>>>>      Op,
>>>>      Opcode, //
>>>>  };
>>>>  use crate::num;
>>>>  
>>>> +/// Defines a schema struct together with its [`Schema`] implementation that decodes into `$target`.
>>>> +///
>>>> +/// Each member of the struct should implement `Schema`. For every (key, index, value) triple
>>>> +/// decoded from the NVKV stream, the generated parent `Schema` implementation will call each member
>>>> +/// in declaration order with that triple. If a member consumes that triple, it will stop there.
>>>> +/// Otherwise it will keep going until all members are tried.
>>>> +///
>>>> +/// The schema struct holds the state required by the schema implementation to do the decode. It's
>>>> +/// recommended to use one of the existing Schema kinds (`Required`, `Accumulated`, `Key`, `Array`,
>>>> +/// `Indexed`) for each member.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// ```
>>>> +/// nvkv_decode! {
>>>> +///     struct RequestSchema => Request {
>>>> +///         id: Required<u32, 0x0001>,
>>>> +///         name: Array<u8, 64, 0x0002>,
>>>> +///     }
>>>> +/// }
>>>> +/// ```
>>>> +macro_rules! nvkv_decode {
>>>> +    (
>>>> +        $(#[$attr:meta])*
>>>> +        $vis:vis struct $name:ident => $target:ident {
>>>> +            $(
>>>> +                $(#[$field_attr:meta])*
>>>> +                $field_vis:vis $field:ident : $ty:ty
>>>> +            ),* $(,)?
>>>> +        }
>>>> +    ) => {
>>>> +        $(#[$attr])*
>>>> +        $vis struct $name {
>>>> +            $(
>>>> +                $(#[$field_attr])*
>>>> +                $field_vis $field: $ty,
>>>> +            )*
>>>> +        }
>>>> +
>>>> +        impl $crate::gsp::nvkv::Schema for $name {
>>>> +            type Target = $target;
>>>> +
>>>> +            fn init() -> impl ::kernel::prelude::Init<Self> {
>>>> +                ::pin_init::init!(Self {
>>>> +                    $( $field <- <$ty as $crate::gsp::nvkv::Schema>::init(), )*
>>>> +                })
>>>> +            }
>>>> +
>>>> +            fn visit(
>>>> +                &mut self,
>>>> +                key: $crate::gsp::nvkv::KeyId,
>>>> +                index: $crate::gsp::nvkv::Index,
>>>> +                value: $crate::gsp::nvkv::DecoderValue<'_>,
>>>> +            ) -> ::kernel::error::Result<bool> {
>>>> +                Ok(false
>>>> +                    $( || $crate::gsp::nvkv::Schema::visit(&mut self.$field, key, index, value)? )*)
>>>
>>> Mmm looks like this is going to be `O(n)` with `n` being the number of
>>> fields?
>>>
>>> This is ok for a first implementation but eventually I hope we can
>>> switch to a more efficient dispatch.
>>
>> I thought quite a bit about this while writing this code, since we need
>> the escape hatch to imperative decode (custom Schema impl basically). To
>> be able to get it down to a match on the key, we need to know ahead of
>> time which keys a Schema will consume. That duplicates the info from the
>> visit() implementation.
>>
>> I thought up a few methods but it's unclear to me which one is best, so
>> I just left it for now. Please LMK if you think this is urgent, I can
>> try in a follow up to improve this. Here are my ideas (when I say O(1)
>> lookup I mean modulo how the compiler decides to do it with the set of
>> key IDs it gets):
>>
>> 1. current code - just visit()
>> pros: key source of truth not duplicates
>> cons: O(field) visit as you say
>>
>> 2. Associated const KEY_ID: Option<KeyId> - None if a Schema accepts multiple keys.
>> You can match on each associated const in the macro.
>> pros: O(1) if the current key goes to a field with KEY_ID = Some(...)
>> cons: O(#fields accepting multiple keys) if current key is one of them
>>
>> 3. fn accepts() -> bool
>> You can match on `if F::accepts(key)` for each field. We could potentially make
>> this const with Gary's const traits polyfill.
>> pros: O(1) if you write an inline-able+optimizable implementation.
>>
>> 4. Associated const KEYS table; use tricks to concat tables
>> pros: O(1) lookup 
>> cons: actually MSRV can't get this to optimize down to O(1) 
>>   if you use slice::contains(), but stable can.
>
> Hmm, am I missing the obvious? Why not generate a `match` expression on IDs of
> fields? It looks like in the example all keys would have a known ID to the
> macro.

Some keys may come from embedded structs, which the macro has no way to
see.