@saiarcot895 could you please review?

@paulmenzel @saiarcot895 could you please help to review this one?
once it is merged, @vadymhlushko-mlnx will have a submodule update and we will take to 202205.

Hmm, is it possible to support both APIs (one where struct psample_metadata is used, and one where the current function signature is used)? Perhaps by having the existing function call a new function that packages the parameters into struct psample_metadata?

Hmm, is it possible to support both APIs (one where struct psample_metadata is used, and one where the current function signature is used)? Perhaps by having the existing function call a new function that packages the parameters into struct psample_metadata?

I don't think it is possible because of the reasons:

1. The 0001-psample-Encapsulate-packet-metadata-in-a-struct.patch and 0002-psample-Add-additional-metadata-attributes.patch patches are taken from upstream, I just integrate them and I don't think it is a good solution to change them.
2. A function psample_sample_packet is used in other vendor's SDK drivers and we can't control them.
3. The best we can do is to implement in the 0003-psample-define-the-macro-PSAMPLE_MD_EXTENDED_ATTR.patch patch a flag PSAMPLE_MD_EXTENDED_ATTR 1 that will signal to Mellanox SDK driver to use a function with struct psample_metadata.

I'm suggesting the following:

1. In 0001-psample-Encapsulate-packet-metadata-in-a-struct.patch, rename psample_sample_packet(struct psample_group *group, struct sk_buff *skb, u32 sample_rate, const struct psample_metadata *md) to psample_sample_packet_new(struct psample_group *group, struct sk_buff *skb, u32 sample_rate, const struct psample_metadata *md)
   • This effectively creates a new function named psample_sample_packet_new that takes in const struct psample_metadata *md instead of the individual attributes.
2. Define a new function psample_sample_packet(struct psample_group *group, struct sk_buff *skb, u32 trunc_size, int in_ifindex, int out_ifindex, u32 sample_rate) that just creates struct psample_metadata and sets out_ifindex, in_ifindex, and trunc_size in that struct, and then calls psample_sample_packet_new.

This will allow existing callers of psample_sample_packet to continue as-is without any changes, while still supporting the newer metadata attributes if the caller calls psample_sample_packet_new. I understand that this won't make it a pure backport of an upstream change anymore, but it will support both calling conventions (especially considering we're not upgrading the kernel here, I think API changes should be limited if possible).

I'm suggesting the following:

1. In 0001-psample-Encapsulate-packet-metadata-in-a-struct.patch, rename psample_sample_packet(struct psample_group *group, struct sk_buff *skb, u32 sample_rate, const struct psample_metadata *md) to psample_sample_packet_new(struct psample_group *group, struct sk_buff *skb, u32 sample_rate, const struct psample_metadata *md)

   • This effectively creates a new function named psample_sample_packet_new that takes in const struct psample_metadata *md instead of the individual attributes.

2. Define a new function psample_sample_packet(struct psample_group *group, struct sk_buff *skb, u32 trunc_size, int in_ifindex, int out_ifindex, u32 sample_rate) that just creates struct psample_metadata and sets out_ifindex, in_ifindex, and trunc_size in that struct, and then calls psample_sample_packet_new.

This will allow existing callers of psample_sample_packet to continue as-is without any changes, while still supporting the newer metadata attributes if the caller calls psample_sample_packet_new. I understand that this won't make it a pure backport of an upstream change anymore, but it will support both calling conventions (especially considering we're not upgrading the kernel here, I think API changes should be limited if possible).

According to the 0002-psample-Add-additional-metadata-attributes.patch the struct psample_metadata was extended with many other parameters alongside with out_ifindex, in_ifindex, and trunc_size:

+        u16 out_tc;
+        u64 out_tc_occ;        /* bytes */
+        u64 latency;        /* nanoseconds */
+        u8 out_tc_valid:1,
+          out_tc_occ_valid:1,
+          latency_valid:1,
+          unused:5;

It means, that if I will implement as you proposed, I anyway need to change a default function signature
from:

 psample_sample_packet(struct psample_group *group, struct sk_buff *skb, u32 trunc_size, int in_ifindex, int out_ifindex, u32 sample_rate)

to:

psample_sample_packet(struct psample_group *group, struct sk_buff *skb, u32 trunc_size, int in_ifindex, int out_ifindex, u32 sample_rate, u16 out_tc, u64 out_tc_occ, u64 latency,u8 out_tc_valid)

Also, it will require me to delete 0003-psample-define-the-macro-PSAMPLE_MD_EXTENDED_ATTR.patch and change our Mellanox-SDK-drivers code.

It means, that if I will implement as you proposed, I anyway need to change a default function signature

Not necessarily. The newly-added attributes don't need to be specified by the caller. The out_tc_valid, out_tc_occ_valid, and latency_valid indicate whether the three new fields contain valid values or not. Additionally, in that patch, there are if-checks added in checking those three *_valid fields first.

This would mean that the function signature would remain the same, but before calling psample_sample_packet_new, the *_valid fields would be set to 0.

@lguohan thoughts on this?

Also, it will require me to delete 0003-psample-define-the-macro-PSAMPLE_MD_EXTENDED_ATTR.patch and change our Mellanox-SDK-drivers code.

I think that patch could remain, but yes, Mellanox SDK driver code would then need to be changed.

Not necessarily. The newly-added attributes don't need to be specified by the caller. The out_tc_valid, out_tc_occ_valid, and latency_valid indicate whether the three new fields contain valid values or not. Additionally, in that patch, there are if-checks added in checking those three *_valid fields first.

This would mean that the function signature would remain the same, but before calling psample_sample_packet_new, the *_valid fields would be set to 0.

@lguohan thoughts on this?

Also, it will require me to delete 0003-psample-define-the-macro-PSAMPLE_MD_EXTENDED_ATTR.patch and change our Mellanox-SDK-drivers code.

I think that patch could remain, but yes, Mellanox SDK driver code would then need to be changed.

I don't understand how it is supposed to work if we need to set 1 to *_valid fields and pass the out_tc, out_tc_occ, and latency fields to the psample_sample_packet function?

For example, the Mellanox-SDK-driver flow - the out_tc, out_tc_occ, latency, out_tc_valid, out_tc_occ_valid, latency_valid fields were calculated before calling the psample_sample_packet function and then those fields has been passed to the psample_sample_packet function as pointer to struct psample_metadata *md.

/easycla

The committers listed above are authorized under a signed CLA.

• ✅ login: vadymhlushko-mlnx / name: Vadym Hlushko (32f7558)