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 15:41:52 +0200
From:   Larysa Zaremba <larysa.zaremba@intel.com>
To:     Stanislav Fomichev <sdf@google.com>
CC:     <bpf@vger.kernel.org>, 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>,
        Jesper Dangaard Brouer <brouer@redhat.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 10/15] ice: Implement VLAN tag hint
Message-ID: <ZGI2oDcWX+o9Ea0T@lincoln>
References: <20230512152607.992209-1-larysa.zaremba@intel.com>
 <20230512152607.992209-11-larysa.zaremba@intel.com>
 <ZF6F+UQlXA9REqag@google.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZF6F+UQlXA9REqag@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?vMiREUBAySwfSNqKr89OUuTovkHON016AdZlHoTkiXy9C2Yy2ZBi6ilUZ8Ys?=
 =?us-ascii?Q?B9hQb/7pVctZ81t/4jxej8wWofrKoVTvFCRB5H6HspNTg1wtKjiAf4GQUNLQ?=
 =?us-ascii?Q?SwoPV1vrBQDJWW474TaYg1s54waxW3GqReqeQ83/a1s7ty0A5mq+Duvmie/S?=
 =?us-ascii?Q?nVt2lR3mI1111YDVAoZ3lbecm8udGL2HnfJF1MNgZOBtRol+ksypZhg3Cv5m?=
 =?us-ascii?Q?wYmfTUk5hEWlZ3gZwbm3vMZdQ0oGdhZJ1GMJ+n5nKuT35Jhyd0Ow6QR3+nkc?=
 =?us-ascii?Q?1NfkVcqNCQdW1jhQNXA0NgLLZGuNl3mvSl4CdDtTEoyD8581VaXlo8WzlQQP?=
 =?us-ascii?Q?pqWGVnot37rGt0aE6yVUZAAEzvG0WOFfoB/d5ZoomJqmT9WmMR5ZUFqhqyan?=
 =?us-ascii?Q?CKn41G7ySyFK9P6jKUOeuPjYEdsMpukJuwwWn+m+ePLh1TybXzoI8oPCx23o?=
 =?us-ascii?Q?CqDjJ5MiQ8Eq7oMuqmh8t217gs+SWOaeDseozmG+cESFy4YgW+CE5UkgevIn?=
 =?us-ascii?Q?wGj9Lq+fTzmlLJw6Hgrp4awFaHmShiQIxHrr/uNfhrtacCie3QVFUHM3UgJy?=
 =?us-ascii?Q?fjgjrJOrsrMbklAdXD/RWCsGejqAkrMpEn/SfsUuBW2+b9NJV/RvLVSMlFQ7?=
 =?us-ascii?Q?5LgYI6JiPnzvc6cH5Jo/0ikR8kr1/kIqwofH23F4Pvri51je6h5+MnuQ49sg?=
 =?us-ascii?Q?J+IVjB3tbf3WO+LDjQP5P/FwiBBJo+6fTLGnNtvKH2nDsFIO/PDeBXtM8dai?=
 =?us-ascii?Q?KrUu4DKDj71nu8UmLNg/IqUq+XPoml4GhBmSkZz/m9ckq8eFvbiyu6cXH00U?=
 =?us-ascii?Q?mJaP2jtpPtYGhHqBjgARNKSiZdohmoBJdaeE5RFj9OzPoCpPp8tZdiIhuzYf?=
 =?us-ascii?Q?qABxvVUhCIGtLxU44iPNly00ioTbt0h6mXL2dm5rFQUY+Bhmjfocqai9L4f4?=
 =?us-ascii?Q?9qs/YvkdDYpCqs7daXACGZFkZ7eilutgNmvsoHdz8reR2UNOH2hLdlHqxrxL?=
 =?us-ascii?Q?PkjXnQO72k/0XGRqZFA+aOT2tv1zVBN26mS9njZZAQsRh6SiJ4L++IczGyAA?=
 =?us-ascii?Q?UY6gZa6kzwNTDiEMkR6D/sTRhLzoNc7GmCYrQ+9/emFGd8bv9dah2+Vef8ZX?=
 =?us-ascii?Q?hJbQDLx4tA5zjHQJ+fMwpOM3xWIq1n9GWg/TseCljU4p3ulJcuWaq2T0+JK/?=
 =?us-ascii?Q?5Phrp8bWDaA20xwqcoKThb2dlzEPwOe08wApzIlUyiVZo4iXBcs0Pje0qzfz?=
 =?us-ascii?Q?OyjaBaYFl8XlJDC/T4wAptLju1ek8obHq6N7q4bfslxjD/kSrNc5kKkOgnjk?=
 =?us-ascii?Q?qFJ2mJyY3AeCsIomabFbIMPF8CbP0c2/GvpsMxeaLQP8/pUdOQbsrURk3c/b?=
 =?us-ascii?Q?JK+EMSHr/1clbbv631evmOdx7ErDX9lhQ9gMId7Am8gGjEni0d7fVJVi9yZ4?=
 =?us-ascii?Q?KkgFgnhuu4usrTiYsh/ojP9hQnzLdsipSmtzI3t1FwILDJ1+lsOxfkGqEWTt?=
 =?us-ascii?Q?khUh5taj2vdiaWFBAt38FsR/A2Pdmyl57xagZwnfUMwaQz7QSJ4OPqXBmMCr?=
 =?us-ascii?Q?L4g5NkqUopdD3Ev946rJvZiwTRtrKkKC0HFJj3jlqZxalkZ0zvhz8oA/7ugV?=
 =?us-ascii?Q?+w=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8ff5423b-be63-413e-615e-08db554a85ee
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5471.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2023 13:44:36.8577
 (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: qYoUdN7cgdSepBfdeSFT+lUWtvrWcSTkVy4lNgkIyBYil3Wd6Q05dmB0fl9l1Zo91o2+yqHPJdOS9+Iwae+KOw2XBF6Bf3+EXngzbBpwXKA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6702
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 Fri, May 12, 2023 at 11:31:21AM -0700, Stanislav Fomichev wrote:
> On 05/12, Larysa Zaremba wrote:
> > Implement .xmo_rx_vlan_tag callback to allow XDP code to read
> > packet's VLAN tag.
> > 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 44 +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> > index 1caa73644e7b..39547feb6106 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
> > @@ -627,7 +627,51 @@ static int ice_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash,
> >  	return 0;
> >  }
> >  
> > +/**
> > + * ice_xdp_rx_ctag - VLAN tag XDP hint handler
> > + * @ctx: XDP buff pointer
> > + * @vlan_tag: destination address
> > + *
> > + * Copy VLAN tag (if was stripped) to the destination address.
> > + */
> > +static int ice_xdp_rx_ctag(const struct xdp_md *ctx, u16 *vlan_tag)
> > +{
> > +	const struct ice_xdp_buff *xdp_ext = (void *)ctx;
> > +	netdev_features_t features;
> > +
> 
> [..]
> 
> > +	features = xdp_ext->rx_ring->netdev->features;
> > +
> > +	if (!(features & NETIF_F_HW_VLAN_CTAG_RX))
> > +		return -EINVAL;
> 
> Passing-by comment: why do we need to check features?
> ice_get_vlan_tag_from_rx_desc seems to be checking a bunch of
> fields in the descriptors, so that should be enough?

Unfortunately, it is not enough, because it only checks, if there is a valid 
value in the descriptor, without distinguishing c-tag from s-tag. In this
hardware, c-tag and s-tag are mutually exclusive, so they can occupy same 
descriptor fields. Checking netdev features is just the easiest way to tell them 
apart.

I guess, storing this information in in the ring structure would be more 
efficient than checking netdev features. I know Piotr Raczynski indends to 
review this series, so maybe he would provide some additional 
feedback/suggestions.

> 
> > +
> > +	*vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc);
> 
> Should we also do the following:
> 
> if (!*vlan_tag)
> 	return -ENODATA;
> 
> ?

Oh, returning VLAN tag with zero value really made sense to me at the beginning,
but after playing with different kinds of packets, I think returning error makes 
more sense. Will change.

> 
> > +	return 0;
> > +}
> > +
> > +/**
> > + * ice_xdp_rx_stag - VLAN s-tag XDP hint handler
> > + * @ctx: XDP buff pointer
> > + * @vlan_tag: destination address
> > + *
> > + * Copy VLAN s-tag (if was stripped) to the destination address.
> > + */
> > +static int ice_xdp_rx_stag(const struct xdp_md *ctx, u16 *vlan_tag)
> > +{
> > +	const struct ice_xdp_buff *xdp_ext = (void *)ctx;
> > +	netdev_features_t features;
> > +
> > +	features = xdp_ext->rx_ring->netdev->features;
> > +
> > +	if (!(features & NETIF_F_HW_VLAN_STAG_RX))
> > +		return -EINVAL;
> > +
> > +	*vlan_tag = ice_get_vlan_tag_from_rx_desc(xdp_ext->eop_desc);
> > +	return 0;
> > +}
> > +
> >  const struct xdp_metadata_ops ice_xdp_md_ops = {
> >  	.xmo_rx_timestamp		= ice_xdp_rx_hw_ts,
> >  	.xmo_rx_hash			= ice_xdp_rx_hash,
> > +	.xmo_rx_ctag			= ice_xdp_rx_ctag,
> > +	.xmo_rx_stag			= ice_xdp_rx_stag,
> >  };
> > -- 
> > 2.35.3
> >