Re: [PATCH RFC 11/20] ctlv: Prepare runtime introspection

[Jason Gunthorpe]

> [ ... 53 lines skipped ... ]
> +/* A cursor packs the record and member where fragmentation resumes. */
> +#define CTLV_INTROSPECTION_MEMBER_SHIFT 48
> +#define CTLV_INTROSPECTION_RECORD_MASK GENMASK_ULL(CTLV_INTROSPECTION_MEMBER_SHIFT - 1, 0)

Use genmask for everything:

 #define CTLV_INTROSPECTION_MEMBER_MASK GENMASK_U64(63, 48)
 #define CTLV_INTROSPECTION_RECORD_MASK GENMASK_U64(47, 0)

	pos->record = FIELD_GET(CTLV_INTROSPECTION_RECORD_MASK, cursor);
	pos->member = FIELD_GET(CTLV_INTROSPECTION_MEMBER_MASK, cursor);

	return FIELD_PREP(CTLV_INTROSPECTION_RECORD_MASK, pos->record) |
	       FIELD_PREP(CTLV_INTROSPECTION_MEMBER_MASK, pos->member);

> +
> +struct ctlv_introspection_dump_position {
> +	u64 record;
> +	u32 member;

Maybe a comment describing how record and member are encoded for each
of the users?

> [ ... 13 lines skipped ... ]
> +/* The immutable root-first chain always has generation zero. */

This comment seems out of place, it should be closer to the 0's it
refers to a function below

> +static bool ctlv_introspection_family_record(struct ctlv_cursor *reply, struct ctlv_cursor *entry,
> +					     const struct ctlv_family *family,
> +					     struct ctlv_introspection_dump_position *pos)
> +{
> +	const struct ctlv_family_schema *schema = family->schema;
> +
> +	if (!ctlv_family_entry_start(reply, entry))
> +		return false;

I'm finding it quite hard to go from this statement to the YAML. I get this:

CTLV_NEST_START(ctlv_family_entry, CTLV_ATTR_FAMILY_ENTRY, CTLV_SCHEMA_NEST)

I asked AI and it said it was this:

attr-sets:
  - name: family-entry
    attrs:
      - family.id
      - family.name

Which is some cyclical reference to this one:

attr-groups:
  # One record per family of the chain a device answers, as reported by
  # families-dump.
  - name: family
    attrs:
      - name: id
        type: uint
      - name: name
        type: string
        max-length: 63
      - name: entry
        attrs: family-entry
        max-occurrences: unlimited

But having poked it some more I see that AI was wrong, and it is the
reverse..

But.. Where is the actual op? I finally see it is here:

ops:
  - name: families-dump
    type: query
    fd-access: read
    reply:
      attrs: family.entry

So.. The op points to family.entry which points back to family.id and
family.name through the attr-sets? I queried AI for a while and came
to this:

  The confusing part is that the family group contains both the leaf
  attributes and the container that holds them. Their grouping
  establishes names and attribute IDs; it doesn't imply
  nesting. Nesting comes from attrs: family-entry.

I don't have a constructive idea yet. I've been trying to understand
the yaml in this series for a while now, and I think it is just
unreadable in this organization. The information is spread about all
over the place and it is very hard to construct an idea of what is
actually in a particular system call. Maybe the missing html
generation would be helpful..

Poking at AI some more I understand it is like this because this is
how the ABI numbers are generated, things need to be in these lists so
they can be automatically numbered.

Going the other way isn't much better..

ops:
  - name: device-get
    type: action
    fd-access: read
    reply:
      attrs: device-reply

Okay, so then I see:

  - name: device-reply
    attrs:
      - device.caps
      - device.info

Uh, why do we have this extra layer? but OK..

attr-groups:
  - name: device
    attrs:
      - name: caps
        attrs: device-caps

Er.. 

  - name: device-caps
    optional-attrs:
      - cap.evidence
      - cap.mr

Getting closer!!

  - name: cap
    attrs:
      - name: evidence
        attrs: evidence-cap

Oh, apparently not..

  - name: evidence-cap
    attrs:
      - cap.min-challenge-length
      - cap.max-challenge-length
      - cap.max-total-output-length
      - cap.object

This time for sure!

  - name: cap
    attrs:
      - name: min-challenge-length
        type: uint

It is too much, this going back and forth to the sets is very
obfuscating. Everything is called attr. There is no editor aid to jump
between the sections, by the time you figure out what type a single
value is you've lost the trail for the rest of it.

This isn't even that complicated, if I ask AI to explain it in a C ish
language:

struct device_get_reply {
	struct {
		optional struct {
			uint min_challenge_length;
			uint max_challenge_length;
			uint max_total_output_length;
			struct {
				enum blob_class;
				string format;
				enum representation;
				uint max_length;
			} objects[];
		} evidence;

		optional struct {
			flags mr_flags; /* read, extend */
		} mr;
	} caps;

	struct {
		/* device-info: filled by the derived family */
	} info;
};

I think the yaml names are not helping, calling everything attrs
everywhere is really confusing. At least the transition into a nest
and an attr-set should be more clearly marked in the yaml..

The yaml would be significantly better if it was layed out according
to the actual semantic flow instead of artificially being twisted into
this need to put everything into a linear list. AI invented this:

device-get:
  request: {}
  reply:
    caps:
      evidence?:
        min-challenge-length: uint
        max-challenge-length: uint
        max-total-output-length: uint
        objects[]:
          blob-class: evidence-blob-class
          format: string
          representation: evidence-representation
          max-length: uint
      mr?:
        mr-flags: mr-device-caps
    info:
      $extend: device-info

So.. how about could we drop the sets, use a direct description of the
layout and do something else for the ID numbering? Maybe the numbering
can be solved directly by only listing the names in:

numbering:
  - device-get.reply
  - device-get.reply.caps
  - device-get.caps.evidence
  -[..]

?

-- 
Jason