Date: Mon, 23 Oct 2023 10:21:53 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, 
	andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org, yhs@...com, 
	john.fastabend@...il.com, kpsingh@...nel.org, haoluo@...gle.com, 
	jolsa@...nel.org, toke@...nel.org, willemb@...gle.com, dsahern@...nel.org, 
	magnus.karlsson@...el.com, bjorn@...nel.org, maciej.fijalkowski@...el.com, 
	hawk@...nel.org, yoong.siang.song@...el.com, netdev@...r.kernel.org, 
	xdp-hints@...-project.net
Subject: Re: [PATCH bpf-next v4 02/11] xsk: Add TX timestamp and TX checksum
 offload support

On 10/20, Jakub Kicinski wrote:
> On Thu, 19 Oct 2023 10:49:35 -0700 Stanislav Fomichev wrote:
> > diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> > index 14511b13f305..22d2649a34ee 100644
> > --- a/Documentation/netlink/specs/netdev.yaml
> > +++ b/Documentation/netlink/specs/netdev.yaml
> > @@ -55,6 +55,19 @@ name: netdev
> >          name: hash
> >          doc:
> >            Device is capable of exposing receive packet hash via bpf_xdp_metadata_rx_hash().
> > +  -
> > +    type: flags
> > +    name: xsk-flags
> > +    render-max: true
> 
> I don't think you're using the MAX, maybe don't render it.
> IDK what purpose it'd serve for feature flag enums.

I was gonna say 'to iterate over every possible bit', but we are using
that 'xxx > 1U << i' implementation (which you also found a bug in).

I can drop it, but the question is: should I drop it from the rest as
well? xdp-act and xdp-rx-metadata have it.

> > +/*
> > + * This structure defines the AF_XDP TX metadata hooks for network devices.
> 
> s/This structure defines the //
> 
> > + * The following hooks can be defined; unless noted otherwise, they are
> > + * optional and can be filled with a null pointer.
> > + *
> > + * void (*tmo_request_timestamp)(void *priv)
> > + *     This function is called when AF_XDP frame requested egress timestamp.
> 
> s/This function is // in many places

SG for this and the one above.

> > + * u64 (*tmo_fill_timestamp)(void *priv)
> > + *     This function is called when AF_XDP frame, that had requested
> > + *     egress timestamp, received a completion. The hook needs to return
> > + *     the actual HW timestamp.
> > + *
> > + * void (*tmo_request_checksum)(u16 csum_start, u16 csum_offset, void *priv)
> > + *     This function is called when AF_XDP frame requested HW checksum
> > + *     offload. csum_start indicates position where checksumming should start.
> > + *     csum_offset indicates position where checksum should be stored.
> > + *
> > + */
> > +struct xsk_tx_metadata_ops {
> > +	void	(*tmo_request_timestamp)(void *priv);
> > +	u64	(*tmo_fill_timestamp)(void *priv);
> > +	void	(*tmo_request_checksum)(u16 csum_start, u16 csum_offset, void *priv);
> > +};
> 
> Could you move the definition of the struct to include/net/xdp_sock.h ?
> netdevice.h doesn't need it.

Let me try..

> > +/* 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)
> 
> Reuse of enum netdev_xsk_flags is not an option?

It is an option, but probably better to keep them separate? Netlink is
for observability, and here have a tighter control over the defines and
UAPI (and the don't have to map 1:1 as in the case of
XDP_TX_METADATA_CHECKSUM_SW, for example).

> > +/* 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)
> 
> Is there a need for this to be on packet-by-packet basis?
> HW issues should generally be fixed by the driver, is there 
> any type of problem in particular you have in mind here?

No, not really, do you think it makes sense to move it to a setsockopt
or something? We'd still have to check it on a per-packet case
though (from xsk_sock), so not sure it is strictly better?

Regarding HW issues: I don't have a good problem in mind, but I
think having a SW path is useful. It least it was useful for me
during developing (to compare the checksum) and I hope it will be
useful for other people as well (mostly as well during development).
Because the API is still a bit complicated and requires getting
pseudo header csum right. Plus the fact that csum_offset is an
offset from csum_start was not super intuitive to me.

> > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > index fe61f85bcf33..5d889c2425fd 100644
> > --- a/net/core/netdev-genl.c
> > +++ b/net/core/netdev-genl.c
> > @@ -14,6 +14,7 @@ netdev_nl_dev_fill(struct net_device *netdev, struct sk_buff *rsp,
> >  		   const struct genl_info *info)
> >  {
> >  	u64 xdp_rx_meta = 0;
> > +	u64 xsk_features = 0;
> 
> rev xmas tree? :)

Oops.

> > +			meta = buffer - xs->pool->tx_metadata_len;
> > +
> > +			if (meta->flags & XDP_TX_METADATA_CHECKSUM) {
> 
> Do we need to worry about reserved / unsupported meta->flags ?

I don't think so, probably not worth the cycles to check for the
unsupported bits? Or do you think it makes sense to clearly return
an error here and this extra check won't actually affect anything?

Powered by blists - more mailing lists