Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 15 May 2023 18:09:55 +0200
From:   Larysa Zaremba <larysa.zaremba@intel.com>
To:     Jesper Dangaard Brouer <jbrouer@redhat.com>
CC:     <bpf@vger.kernel.org>, <brouer@redhat.com>,
        Stanislav Fomichev <sdf@google.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        "Tony Nguyen" <anthony.l.nguyen@intel.com>,
        Anatoly Burakov <anatoly.burakov@intel.com>,
        Alexander Lobakin <alexandr.lobakin@intel.com>,
        Magnus Karlsson <magnus.karlsson@gmail.com>,
        Maryam Tahhan <mtahhan@redhat.com>,
        <xdp-hints@xdp-project.net>, <netdev@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RESEND bpf-next 09/15] xdp: Add VLAN tag hint
Message-ID: <ZGJZU89AK/3mFZXW@lincoln>
References: <20230512152607.992209-1-larysa.zaremba@intel.com>
 <20230512152607.992209-10-larysa.zaremba@intel.com>
 <b0694577-e2b3-f6de-cf85-aed99fdf2496@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <b0694577-e2b3-f6de-cf85-aed99fdf2496@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?KuLV7sJDb9JjlPNV6uwpF4AOwe5xd0UfvQCyr9pMHmd6jeO5cJpQ+2jqAExX?=
 =?us-ascii?Q?kwaETvl/KVezxRKROhjMYlVFu1lCM4fWkv/DcvYROcpEEP7iWwaA/Q6i1iGc?=
 =?us-ascii?Q?m++rdyYbZVHbBLGGC7U2bzlAsPGpAa00D6JIV1sO9p9kMw5ZBqi3xV1UA1Hi?=
 =?us-ascii?Q?7RMi4bywxPg2tStX/D58Y4+LcmYodF6+JlE7M+R4mV2RTg2st19mOQcKMAhX?=
 =?us-ascii?Q?Y14DmcT2oODvy9dYdv+fX+tcRUbC6vbpOKuDWNXS90xERtP0UVFB72YlMCNG?=
 =?us-ascii?Q?xZXixZ5fDeX7azATcdSJnlo12fUsAAS2MTzFrx3gRlywV/EY+B6mGw9Dl03w?=
 =?us-ascii?Q?cHQA2XaOcVXHNLWuHwvV6g+T8vNXwwOBz7IiYfCF/+L53RpWIBJdFClKhrMX?=
 =?us-ascii?Q?eZZj81Ungkop32wg78XadPO6s4BmeDyg4W+TsuEPnga5MCRbS17EB7L+jpfW?=
 =?us-ascii?Q?xvfywuHI+bNDcqrAlQMmZbLAHTpYoa2V9/6XG3viDhJaqbTYUQSnXml1qrb6?=
 =?us-ascii?Q?fsUJGV3x509YD0oSRytxErwoq5hjnKiMJnMKGiioFazNPEtOp2n6Zk8QmY96?=
 =?us-ascii?Q?n0aZBVMBeS0WVWwV6DVfkFLb2wmiE1n4+g/yRwQSgEuLwOy4FH04FEIPxUYw?=
 =?us-ascii?Q?HeTUsC1ST+zUjdB9vUxNFzasGAhVxUz/+bkhBYjrNzInu3KaCY9w8VP5C2Kb?=
 =?us-ascii?Q?iCN4QHQVlmZP09D7wss5rcB9ptIrU7AZFZ9bwEvO1GnekDKlHWx8JFsu4Ixf?=
 =?us-ascii?Q?9V1NbVOc54GlxnoQq/VskNNA1QzEhCJF0E5JGjwSJEPsqXq4bDR4UPY+HtGR?=
 =?us-ascii?Q?k70NYxxJLTEi6KFqWfXDOepUVGEny5DRhS1wok6Cdpv+0drnKZs9vVfz3KG/?=
 =?us-ascii?Q?cv0ZoSMfnOky+uCUxJ3iYXEglt54bcIEwojGPBkeJhHMWsvI3+L6QzS19xoA?=
 =?us-ascii?Q?FlxbjJ78gdEGOI5YdH54O9QJC+GWXvLWXDyug1DfEtPE5BTbrlQEP5LjFgds?=
 =?us-ascii?Q?LTDC5MpIMf7TAFZ4BiB8MEWmR22xNKrty/ZQnuGS1kv65VZs7saMohTHaSOt?=
 =?us-ascii?Q?UgYvQHuICwwJLJVTPPAI/sl2FWmRxlpA7I9ZxoPpnvQvQzvfWd+gNfk2ZtZf?=
 =?us-ascii?Q?9FboSomek/FbzOnQZUnN8HZB6NTy44cWcEjn2J6iwRG+H3pv+POI5qyuDSLD?=
 =?us-ascii?Q?OimkLhF4ZC/pd2QRiKdZO2FLDnysPX+rlnTaE3DJgdCDqsnotpptWxi+Oq4+?=
 =?us-ascii?Q?YL+S1mUANPDkCSRBxEarlCpU37yl0avDdtnyzmr3LXyqUf1Wtkc3kIrtAnTN?=
 =?us-ascii?Q?SHIlRGWRVI6vV3Kp4ierMaxjifl5ghPj+noSGRa3CO4NFzT+kRo/u5Ptn4Qm?=
 =?us-ascii?Q?dBtSwt4tw1yBPHlhOvcWi2nwvXLwBT4BlDKrU/aKTzN9Mcr4maRpbsBtD7cT?=
 =?us-ascii?Q?PLf7c8Ywvba4tB60uBlBQ1E00Gt30POuhKpf9qTQfjTKjLg4ykY9tnxvgroo?=
 =?us-ascii?Q?jTc8nemnIN5aOuo8VEm3DmenwouL5gK44gUdPFxAnNaxZUp9VgXXIsYRp4/s?=
 =?us-ascii?Q?NOji/N4jQLzb0XFzqPO/tsLmzmk6GpSqA+rA6mn52Gjt1MR88wbhKJFCtnq6?=
 =?us-ascii?Q?og=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b83d99b-3787-4d75-de9b-08db555f34f2
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5471.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2023 16:12:40.3987
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: PrN5DaY1K21lBtRz83TIKy4CYroTXz79ULLnOVQOurESjw3NvxdGoUBUYwKpD826jmQgVhYk+qFIo1WU+RReBWcin4zCWQoRqNwlPgVKopc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5483
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, May 15, 2023 at 05:36:12PM +0200, Jesper Dangaard Brouer wrote:
> 
> 
> On 12/05/2023 17.26, Larysa Zaremba wrote:
> > Implement functionality that enables drivers to expose VLAN tag
> > to XDP code.
> > 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> [...]
> 
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 41e5ca8643ec..eff21501609f 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -738,6 +738,30 @@ __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash,
> >   	return -EOPNOTSUPP;
> >   }
> 
> Remember below becomes part of main documentation on HW metadata hints:
>  - https://kernel.org/doc/html/latest/networking/xdp-rx-metadata.html
> 
> Hint compiling locally I use:
>  make SPHINXDIRS="networking" htmldocs
> 
> > +/**
> > + * bpf_xdp_metadata_rx_ctag - Read XDP packet inner vlan tag.
> 
> Is bpf_xdp_metadata_rx_ctag a good function name for the inner vlan tag?
> Like wise below "stag".
> 
> I cannot remember if the C-tag or S-tag is the inner or outer vlan tag.
> 
> When reading BPF code that use these function names, then I would have
> to ask Google for help, or find-and-read this doc.
> 
> Can we come-up with a more intuitive name, that e.g. helps when reading
> the BPF-prog code?

Well, my reasoning for such naming is that if someone can configure s-tag 
stripping in ethtool with 'rx-vlan-stag-hw-parse', they shouldn't have any 
problem with understanding those function names.

One possible improvement that comes to mind is maybe (similarly ethtool) calling 
c-tag just 'tag' and letting s-tag stay 'stag'. Because c-tag is this default 
802.1q tag, which is supported by various hardware, while s-tag is significantly 
less widespread.

But there are many options, really.

What are your suggestions?

> 
> > + * @ctx: XDP context pointer.
> > + * @vlan_tag: Return value pointer.
> > + *
> 
> IMHO right here, there should be a description.
> 
> E.g. for what a VLAN "tag" means.  I assume a "tag" isn't the VLAN id,
> but the raw VLAN tag that also contains the prio numbers etc.
> 
> It this VLAN tag expected to be in network-byte-order ?
> IMHO this doc should define what is expected (and driver devel must
> follow this).

Will specify that.

> 
> > + * Returns 0 on success or ``-errno`` on error.
> > + */
> > +__bpf_kfunc int bpf_xdp_metadata_rx_ctag(const struct xdp_md *ctx, u16 *vlan_tag)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +/**
> > + * bpf_xdp_metadata_rx_stag - Read XDP packet outer vlan tag.
> > + * @ctx: XDP context pointer.
> > + * @vlan_tag: Return value pointer.
> > + *
> > + * Returns 0 on success or ``-errno`` on error.
> 
> IMHO we should provide more guidance to expected return codes, and what
> they mean.  IMHO driver developers must only return codes that are
> described here, and if they invent a new, add it as part of their patch.

That's a good suggestion, I will expand the comment to describe error codes used 
so far.

> 
> See, formatting in bpf_xdp_metadata_rx_hash and check how this gets
> compiled into HTML.
> 
> 
> > + */
> > +__bpf_kfunc int bpf_xdp_metadata_rx_stag(const struct xdp_md *ctx, u16 *vlan_tag)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
>