On 10/20, Alexei Starovoitov wrote:
> On Thu, Oct 19, 2023 at 10:49:35AM -0700, Stanislav Fomichev wrote:
> > diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
> > index 2ecf79282c26..ecfd67988283 100644
> > --- a/include/uapi/linux/if_xdp.h
> > +++ b/include/uapi/linux/if_xdp.h
> > @@ -106,6 +106,43 @@ struct xdp_options {
> > #define XSK_UNALIGNED_BUF_ADDR_MASK \
> > ((1ULL << XSK_UNALIGNED_BUF_OFFSET_SHIFT) - 1)
> >
> > +/* Request transmit timestamp. Upon completion, put it into tx_timestamp
> > + * field of struct xsk_tx_metadata.
> > + */
> > +#define XDP_TX_METADATA_TIMESTAMP (1 << 0)
> > +
> > +/* Request transmit checksum offload. Checksum start position and offset
> > + * are communicated via csum_start and csum_offset fields of struct
> > + * xsk_tx_metadata.
> > + */
> > +#define XDP_TX_METADATA_CHECKSUM (1 << 1)
> > +
> > +/* Force checksum calculation in software. Can be used for testing or
> > + * working around potential HW issues. This option causes performance
> > + * degradation and only works in XDP_COPY mode.
> > + */
> > +#define XDP_TX_METADATA_CHECKSUM_SW (1 << 2)
> > +
> > +struct xsk_tx_metadata {
> > + union {
> > + struct {
> > + __u32 flags;
> > +
> > + /* XDP_TX_METADATA_CHECKSUM */
> > +
> > + /* Offset from desc->addr where checksumming should start. */
> > + __u16 csum_start;
> > + /* Offset from csum_start where checksum should be stored. */
> > + __u16 csum_offset;
> > + };
> > +
> > + struct {
> > + /* XDP_TX_METADATA_TIMESTAMP */
> > + __u64 tx_timestamp;
> > + } completion;
> > + };
> > +};
>
> Could you add a comment to above union that csum fields are consumed by the driver
> before it xmits the packet while timestamp is filled during xmit, so union
> doesn't prevent using both features simultaneously.
> It's clear from the example, but not obvious from uapi and the doc in patch 11
> doesn't clarify it either.
>
> Also please add a name to csum part of the union like you did for completion.
> We've learned it the hard way with bpf_attr. All anon structs better have field name
> within a union. Helps extensibility (avoid conflicts) in the long term.
Sure, will do, thanks!
> Other than this the patch set looks great to me.
> With Saeed and Magnus acks we can take it in.
Magnus is on CC, so I hope see sees the request.
Added Saeed here as well. Saeed, can you please take a look at the mlx part?
You've been on CC for a particular patch, but just in case:
https://lore.kernel.org/bpf/20231019174944.3376335-5-sdf@xxxxxxxxxx/T/#u
