From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A490EC433FE for ; Fri, 18 Nov 2022 18:19:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242647AbiKRSTO (ORCPT ); Fri, 18 Nov 2022 13:19:14 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58470 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S242623AbiKRSTE (ORCPT ); Fri, 18 Nov 2022 13:19:04 -0500 Received: from mail-oa1-x31.google.com (mail-oa1-x31.google.com [IPv6:2001:4860:4864:20::31]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B6A1E922D9 for ; Fri, 18 Nov 2022 10:18:56 -0800 (PST) Received: by mail-oa1-x31.google.com with SMTP id 586e51a60fabf-142612a5454so6239264fac.2 for ; Fri, 18 Nov 2022 10:18:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=fNnwGanczIlOEoVy4OEmoR/dSYWkyWUH5BAq5Ob+Kiw=; b=gJBrl3WZ97qVM3NI2LxOAIkcLzX+1KjC9HfQrZhs+yxO7EeHBYC0NOA8ndwqQIu9m9 MzsuBWsITSp9sLNXnWnwi8naaGOehc9PKEomLULvmyYR0LVTjOgvfoBOwExTadH5bycM ED6yT1CxxGG5yT8Ihb9SmELwyOHU0P7PHFfQpSsH0rSJchYYT5rYPa/qoe7Eg6uKlboE z1T6s7Qdp/DJm3NBAt4hXF16NjB7CW8aAB2kXkctLW1w+x96U0STNaBaAvjnai3NxfiS gIIwlMz9ItIe8AN7iPP/Oa2bPTelXB6lihZ6sqBdThz6xN+JiEbNfe02mmxZjyp8+sBW f0dg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=fNnwGanczIlOEoVy4OEmoR/dSYWkyWUH5BAq5Ob+Kiw=; b=rA15ri0cuk5BH1T7W9lDh66l4R15hxI14yZF5wryyYYcVvYs0AUtfEcthaSxAP4x6C dYAPNwH5xXBSyqMT7dEkO8Ml9dKO4INF8Oe5BEYDvQlsKdw5SOva8igvJ8CHL4y9jpwC 1MY02fO97efaRmcVZCGZtSwN36TglW9hIqs7OaCAIb3PBAIxvXmoyL6Bj9E/HYhnIBEJ MtWBxHm9gYqRBl0j6LFtY3f4GsROc6K+UnZhmUg19MKefmwCZlXS9AoVXUaiSJERYY2O R1+fI+VRxp3A31kpXwVP5pPY/het44Z5HKl2sOmRrlmazY8/Xv1JFz/rQkZdMSZbALyd dp3g== X-Gm-Message-State: ANoB5pn1JKjvLziC8cz2K3QYBF00lp+VkoCRjMKkJZPSWMBYslrQmGNW SGD4J7IYbFFehYLw+gtmfpXrYl+M7rE3g4fkscfhoA== X-Google-Smtp-Source: AA0mqf5Ar+kY1e8eGdghMnj0xIaR0R7Fv+/GgQjxHkmHfBRbquNZ+pOSQ6UBsy2yKGR7QhNo6JqfeLia+RddEIiT4II= X-Received: by 2002:a05:6870:9d95:b0:13b:a163:ca6 with SMTP id pv21-20020a0568709d9500b0013ba1630ca6mr8143380oab.125.1668795535941; Fri, 18 Nov 2022 10:18:55 -0800 (PST) MIME-Version: 1.0 References: <20221115030210.3159213-1-sdf@google.com> <20221115030210.3159213-7-sdf@google.com> In-Reply-To: From: Stanislav Fomichev Date: Fri, 18 Nov 2022 10:18:44 -0800 Message-ID: Subject: Re: [PATCH bpf-next 06/11] xdp: Carry over xdp metadata into skb context To: Jesper Dangaard Brouer Cc: bpf@vger.kernel.org, brouer@redhat.com, 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, David Ahern , Jakub Kicinski , Willem de Bruijn , Anatoly Burakov , Alexander Lobakin , Magnus Karlsson , Maryam Tahhan , xdp-hints@xdp-project.net, netdev@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On Fri, Nov 18, 2022 at 6:05 AM Jesper Dangaard Brouer wrote: > > > On 15/11/2022 04.02, Stanislav Fomichev wrote: > > Implement new bpf_xdp_metadata_export_to_skb kfunc which > > prepares compatible xdp metadata for kernel consumption. > > This kfunc should be called prior to bpf_redirect > > or when XDP_PASS'ing the frame into the kernel (note, the drivers > > have to be updated to enable consuming XDP_PASS'ed metadata). > > > > veth driver is amended to consume this metadata when converting to skb. > > > > Internally, XDP_FLAGS_HAS_SKB_METADATA flag is used to indicate > > whether the frame has skb metadata. The metadata is currently > > stored prior to xdp->data_meta. bpf_xdp_adjust_meta refuses > > to work after a call to bpf_xdp_metadata_export_to_skb (can lift > > this requirement later on if needed, we'd have to memmove > > xdp_skb_metadata). > > > > I think it is wrong to refuses using metadata area (bpf_xdp_adjust_meta) > when the function bpf_xdp_metadata_export_to_skb() have been called. > In my design they were suppose to co-exist, and BPF-prog was expected to > access this directly themselves. > > With this current design, I think it is better to place the struct > xdp_skb_metadata (maybe call it xdp_skb_hints) after xdp_frame (in the > top of the frame). This way we don't conflict with metadata and > headroom use-cases. Plus, verifier will keep BPF-prog from accessing > this area directly (which seems to be one of the new design goals). > > By placing it after xdp_frame, I think it would be possible to let veth > unroll functions seamlessly access this info for XDP_REDIRECT'ed > xdp_frame's. > > WDYT? Not everyone seems to be happy with exposing this xdp_skb_metadata via uapi though :-( So I'll drop this part in the v2 for now. But let's definitely keep talking about the future approach. Putting it after xdp_frame SGTM; with this we seem to avoid the need to memmove it on adjust_{head,meta}. But going back to the uapi part, what if we add separate kfunc accessors for skb exported metadata? To export: bpf_xdp_metadata_export_rx_timestamp_to_skb(ctx, rx_timestamp) bpf_xdp_metadata_export_rx_hash_to_skb(ctx, rx_hash) // ^^ these prepare xdp_skb_metadata after xdp_frame, but not expose it via uapi/af_xdp/etc Then bpf_xdp_metadata_export_to_skb can be 'static inline' define in the headers: void bpf_xdp_metadata_export_to_skb(ctx) { if (bpf_xdp_metadata_rx_timestamp_supported(ctx)) bpf_xdp_metadata_export_rx_timestamp_to_skb(ctx, bpf_xdp_metadata_rx_timestamp(ctx)); if (bpf_xdp_metadata_rx_hash_supported(ctx)) bpf_xdp_metadata_export_rx_hash_to_skb(ctx, bpf_xdp_metadata_rx_hash(ctx)); } We can also do the accessors: u64 bpf_xdp_metadata_skb_rx_timestamp(ctx) u32 bpf_xdp_metadata_skb_rx_hash(ctx) Hopefully we can unroll at least these, since they are not part of the drivers, it should be easier to argue... The only issue, it seems, is that if the final bpf program would like to export this metadata to af_xdp, it has to manually adj_meta and use bpf_xdp_metadata_skb_rx_xxx to prepare a custom layout. Not sure whether performance would suffer with this extra copy; but we can at least try and see..