From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-x64a.google.com (mail-pl1-x64a.google.com [IPv6:2607:f8b0:4864:20::64a]) by mail.toke.dk (Postfix) with ESMTPS id D1270A34D79 for ; Mon, 23 Oct 2023 19:21:56 +0200 (CEST) Authentication-Results: mail.toke.dk; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20230601 header.b=ESNVF/b6 Received: by mail-pl1-x64a.google.com with SMTP id d9443c01a7336-1c9da175faaso23387825ad.1 for ; Mon, 23 Oct 2023 10:21:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1698081715; x=1698686515; darn=xdp-project.net; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=/B8lageGMlr9qUG/KlHIRLTO+L0Q69aSFsg/BovmTxk=; b=ESNVF/b6iuzwPsHeJ6vFbhmPouFN9tcnDfK9ERdWPGqBNjek/HZvCYB9zpaMvyZqT7 6x20ecy1njqrM7SqL4c7UV6y9Jui6O/3RihwMtvFKV9I4MHRr2GhYC1oWhTqFmX7om/S Wf8IauV2KWACTnnfF1lCtf/+9WKyFNqmcuI8q8UdpKn+LSzJQczGS4KsO8vqgHxHhRjB 4sR7Dp+kTcATNnN0tICZeQW+6jATEOpNlwn+8Eb8VhB0RY/6ntsA1nBFd2HwqsBulEa4 a75l6qAKR3Boi7daxNaYf8t0ppoV3CnzVf3aAIwWHNZDfviKHX5yPk543j66byhlK1+Z kc2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698081715; x=1698686515; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=/B8lageGMlr9qUG/KlHIRLTO+L0Q69aSFsg/BovmTxk=; b=q4/U4Ui9+qXDVQdaTcqkS+qfNgINt9426gWryDIYQ79eMsYPGT9GcBDcTwJ/OZJNbE livmYQc4GDYEvK2U9RNOCSrJRq3f7cef+zE2dtqg/gwTgnx07+I0wFF71f7u1Cv2LQFZ O7jAhYPpSa4lQO9M54z1cz5HVqkS7SSqZ4XW9ktGgndtN9ktJduDM2XQ1wAqm4Y/cTpK jVZlYfGyAlxfljdmHnFE7dHz3XELKBtP55EKePN6p1bkm4Aijv0Xh7NITjpXpzfvNYVS 0Jh/Y5CdLgkDxlhmYddlryxZluHtPerqTL7VGK2CZf+ciSdXPiHGb8ZpKFv0YUmdeK62 ZVzg== X-Gm-Message-State: AOJu0YwKJD6rrZ7CLz8dzuHqkEQuMVTiUCzavu5Ar6H2qI/3ed/yIlIr bqgzVzTYE5npac24hVGqtuMkd1I= X-Google-Smtp-Source: AGHT+IFKjYhxdkK+MF/cAYrA5yautwocYEWjQfJGCaxzpBQlEwcsM5B9nN/miZXqgukOUHGRKsh+0o8= X-Received: from sdf.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5935]) (user=sdf job=sendgmr) by 2002:a17:902:654d:b0:1c9:d366:8f11 with SMTP id d13-20020a170902654d00b001c9d3668f11mr167418pln.7.1698081714824; Mon, 23 Oct 2023 10:21:54 -0700 (PDT) Date: Mon, 23 Oct 2023 10:21:53 -0700 In-Reply-To: <20231020180411.2a9f573d@kernel.org> Mime-Version: 1.0 References: <20231019174944.3376335-1-sdf@google.com> <20231019174944.3376335-3-sdf@google.com> <20231020180411.2a9f573d@kernel.org> Message-ID: From: Stanislav Fomichev To: Jakub Kicinski Content-Type: text/plain; charset="utf-8" Message-ID-Hash: 6CS67CPM4NYEZWVJ7RWY3J4LJXMUZBKX X-Message-ID-Hash: 6CS67CPM4NYEZWVJ7RWY3J4LJXMUZBKX X-MailFrom: 3sqs2ZQMKCRUDy019916z.x97IyA-238EDIyA-AC94zxE.8zE@flex--sdf.bounces.google.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org, martin.lau@linux.dev, song@kernel.org, yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org, haoluo@google.com, jolsa@kernel.org, toke@kernel.org, willemb@google.com, dsahern@kernel.org, magnus.karlsson@intel.com, bjorn@kernel.org, maciej.fijalkowski@intel.com, hawk@kernel.org, yoong.siang.song@intel.com, netdev@vger.kernel.org, xdp-hints@xdp-project.net X-Mailman-Version: 3.3.8 Precedence: list Subject: [xdp-hints] Re: [PATCH bpf-next v4 02/11] xsk: Add TX timestamp and TX checksum offload support List-Id: XDP hardware hints design discussion Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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?