On 11/10/22 3:29 PM, Toke Høiland-Jørgensen wrote:
For the metadata consumed by the stack right now it's a bit
hypothetical, yeah. However, there's a bunch of metadata commonly
supported by hardware that the stack currently doesn't consume and that
hopefully this feature will end up making more accessible. My hope is
that the stack can also learn how to use this in the future, in which
case we may run out of space. So I think of that bit mostly as
future-proofing...
ic. in this case, Can the btf_id be added to 'struct xdp_to_skb_metadata' later
if it is indeed needed? The 'struct xdp_to_skb_metadata' is not in UAPI and
doing it with CO-RE is to give us flexibility to make this kind of changes in
the future.
My worry is mostly that it'll be more painful to add it later than just
including it from the start, mostly because of AF_XDP users. But if we
do the randomisation thing (thus forcing AF_XDP users to deal with the
dynamic layout as well), it should be possible to add it later, and I
can live with that option as well...
imo, considering we are trying to optimize unnecessary field initialization as
below, it is sort of wasteful to always initialize the btf_id with the same
value. It is better to add it in the future when there is a need.
We should probably also have a flag set on the xdp_frame so the stack
knows that the metadata area contains relevant-to-skb data, to guard
against an XDP program accidentally hitting the "magic number" (BTF_ID)
in unrelated stuff it puts into the metadata area.
Yeah, I think having a flag is useful. The flag will be set at xdp_buff and
then transfer to the xdp_frame?
Yeah, exactly!
After re-reading patch 6, have another question. The 'void
bpf_xdp_metadata_export_to_skb();' function signature. Should it at
least return ok/err? or even return a 'struct xdp_to_skb_metadata *'
pointer and the xdp prog can directly read (or even write) it?
Hmm, I'm not sure returning a failure makes sense? Failure to read one
or more fields just means that those fields will not be populated? We
should probably have a flags field inside the metadata struct itself to
indicate which fields are set or not, but I'm not sure returning an
error value adds anything? Returning a pointer to the metadata field
might be convenient for users (it would just be an alias to the
data_meta pointer, but the verifier could know its size, so the program
doesn't have to bounds check it).
If some hints are not available, those hints should be initialized to
0/CHECKSUM_NONE/...etc.
The problem with that is that then we have to spend cycles writing
eight bytes of zeroes into the checksum field :)
With a common 'struct xdp_to_skb_metadata', I am not sure how some of these zero
writes can be avoided. If the xdp prog wants to optimize, it can call
individual kfunc to get individual hints.
Erm, we just... don't write those fields? Something like:
void write_skb_meta(hw, ctx) {
struct xdp_skb_metadata meta = ctx->data_meta - sizeof(struct xdp_skb_metadata);
meta->valid_fields = 0;
if (hw_has_timestamp) {
meta->timestamp = hw->timestamp;
meta->valid_fields |= FIELD_TIMESTAMP;
} /* otherwise meta->timestamp is just uninitialised */
if (hw_has_rxhash) {
meta->rxhash = hw->rxhash;
meta->valid_fields |= FIELD_RXHASH;
} /* otherwise meta->rxhash is just uninitialised */
...etc...
}
Ah, got it. Make sense. My mind was stalled in the paradigm that a helper that
needs to initialize the result.
