From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-xb49.google.com (mail-yb1-xb49.google.com [IPv6:2607:f8b0:4864:20::b49]) by mail.toke.dk (Postfix) with ESMTPS id D8588A34E76 for ; Mon, 23 Oct 2023 20:46:48 +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=xNr8wMI1 Received: by mail-yb1-xb49.google.com with SMTP id 3f1490d57ef6-d8486b5e780so4459753276.0 for ; Mon, 23 Oct 2023 11:46:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1698086807; x=1698691607; 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=aWCt7fVifu9lSRwvkCzaL8k9JM3Sqd8iOqTHPjTp+Z8=; b=xNr8wMI1BLjfVE/pjiE1kPixuNy77gVf9uR+hoJdBUIeC67eQkAyvm2Uds/cOxAz4q MwypV2Sv7KamnlzrZxeBWl858RBrc+RqLY+eipX/FvvH2PNPNtplpQmQeG+ArwZlV1+E d4D43CiWpDtWlIp0m5O37e4WJb3CVOSFQxP5mbH8pd3W3TlLiRtaV6ojq3CwW/6ds5Um 1k1UgpHmBHtElT1hncAOW1ZKAUEiuGjwBKPeKkECptJFh7UvwNTohyUpra3fk5ibtsEu VsvzzEKSKKagMrhHvwYAcmaJTuOJvuBQffuqjyP3OrNazCicwtK9YKc4U6lHguI0Q1Sy YdpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698086807; x=1698691607; 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=aWCt7fVifu9lSRwvkCzaL8k9JM3Sqd8iOqTHPjTp+Z8=; b=Mz3My77uXUGSWwaKA9byB/ZGl6JdClVw9HwwWrqxGGPVRVjEpWpbNbDOrML08ffDoJ 1MwhRF/EEy/EMp5fZiRFbpKx098VMxfX2OdbXTJGUAbkxDVhNOpChEIX1kxEq1AZfwzy z2sPvu+jHcQqGiDfdNy4yN5KzbSoGVlWT/Hx/yATqaZ5v0+zUnUMrpxuxWhcG9abquA+ uHFE60Dq5FGwe9Ms+EL2P54NHFvpLLKOomPPvV1LopoIt24GeWP12KG93lw4S6UhbgD0 W2R405cmv9EejAe8mNAMYKYrqKvRn+drrSflJs0Xvzc1BjdB/BGesQwPGVcxERuWVCmd E42A== X-Gm-Message-State: AOJu0YzqDBHwqZS1eiGQa6kWvVlat35yQNcuai8KLt2/wbsXi9ilPZKC uVQm1rFTMOZ+q9SYEvcJ3ijr68E= X-Google-Smtp-Source: AGHT+IHVcMG7X5PXY5nV8TbWav2pfkXl0AU0B4K3H28H7klf2hO78/4IOCsT2NNurl+37A28QdkmVt4= X-Received: from sdf.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5935]) (user=sdf job=sendgmr) by 2002:a25:374e:0:b0:d9a:425c:f5c with SMTP id e75-20020a25374e000000b00d9a425c0f5cmr183992yba.1.1698086807648; Mon, 23 Oct 2023 11:46:47 -0700 (PDT) Date: Mon, 23 Oct 2023 11:46:46 -0700 In-Reply-To: <20231023111209.2278899c@kernel.org> Mime-Version: 1.0 References: <20231019174944.3376335-1-sdf@google.com> <20231019174944.3376335-3-sdf@google.com> <20231020180411.2a9f573d@kernel.org> <20231023111209.2278899c@kernel.org> Message-ID: From: Stanislav Fomichev To: Jakub Kicinski Content-Type: text/plain; charset="utf-8" Message-ID-Hash: BWRIFZ7OKR2L7N5DRRV5SRQCPWSRTU42 X-Message-ID-Hash: BWRIFZ7OKR2L7N5DRRV5SRQCPWSRTU42 X-MailFrom: 3l782ZQMKCSIQBDEMMEJC.AMKVBN-FGLRQVBN-NPMHCAR.LCR@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/23, Jakub Kicinski wrote: > On Mon, 23 Oct 2023 10:21:53 -0700 Stanislav Fomichev wrote: > > 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. > > The xdp-act one looks used. xdp-rx-metadata looks unused, so you could > drop. But up to you if you want to clean it up. Ok. I'll cleanup xdp-rx-metadata in the same path. Might we worth it to limit copy-paste spread.. > > > > +/* 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). > > The duplication is rather apparent, and they are flags so compiler > can't help us catch misuses of one set vs the other. > > If you prefer to keep the separate defines - I'd rename them to tie > them to the field more strongly. Specifically they should have the > word "flags" in them? > > XDP_TXMD_FLAGS_TIMESTAMP > XDP_TXMD_FLAGS_CHECKSUM > > maybe? Sg, will rename. > > > > +/* 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? > > Setsockopt or just ethtool -K $ifc tx off ? And check device features? > Maybe I'm overly sensitive but descriptor bits are usually super > precious :) Good point on the descriptor bits. Let me try to move to a setsockopt. > > 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. > > Okay, I'm not strongly opposed, I just wanted to flag it. > If nobody else feels the same way, and you like the separate bit - > perfectly fine by me. > > > > > + 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? > > Hm, it is uAPI, isn't it? We try to validate anything kernel gets these > days, why would the flags be different? Shouldn't be more than 2 cycles. Yeah, agreed, worst case we can have some static_branch to disable it. But fair point that unlikely we'll see it cause any issues.