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 17:45:54 +0200
From:   Larysa Zaremba <larysa.zaremba@intel.com>
To:     Jesper Dangaard Brouer <jbrouer@redhat.com>
CC:     Stanislav Fomichev <sdf@google.com>, <brouer@redhat.com>,
        <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>,
        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: <ZGJTsgmrvylUH9cY@lincoln>
References: <20230512152607.992209-1-larysa.zaremba@intel.com>
 <20230512152607.992209-11-larysa.zaremba@intel.com>
 <ZF6F+UQlXA9REqag@google.com>
 <ZGI2oDcWX+o9Ea0T@lincoln>
 <e4a9fa43-06f7-5271-effc-20cac59b0e64@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <e4a9fa43-06f7-5271-effc-20cac59b0e64@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?lc5vbAsLotNMDVYSysp/hJFv6ZPFc8zOK06mDJsB0GiJDvzRIiAuZX80Blu0?=
 =?us-ascii?Q?WNVvV6xLSG+gO/JwdjEFEYDFwR4ICl+DEcqeFvnwsy9q2NXFYEFFZkRSPXIG?=
 =?us-ascii?Q?GBKxK8XiynlVLQ2rJP3HgNRmafAgPCqfMJGICI8L2ih5OAX0fLylsDan9VxG?=
 =?us-ascii?Q?k1dO8XCuZgs0T2EomvFCKoCbeFPLn9PudC8BCzScxHM2Np7tnvfgE32PU+UK?=
 =?us-ascii?Q?bGiic294+wd3IemAo5QdVrP/I6278PHwGbs9x70I+ItnhYtHeWbvclFfsQw5?=
 =?us-ascii?Q?KqVFJCoetHGp49vztz32lpE3hnCwcLB4fsV/m5VcOljRdC0B3usSiNIphOM0?=
 =?us-ascii?Q?D/gvHh9wnr2G0f1lDCJYKfl+o53Sdno5gQBJbalnveIXVvFcf4W7zEIqkq/v?=
 =?us-ascii?Q?KOg2lksmxtXFn0VxhC/ZtlgTnBSJm/cdLgo85AE6cW8QDIsL7M/tczQA2LS5?=
 =?us-ascii?Q?1zL16o9uW4BixKg8GxCI7dDJn+Pbz7ylfD95q7Cem3eKmS6un8r6T53erixt?=
 =?us-ascii?Q?dwOsQliepBbsv/55Os71q08/cC48yis3yTBfxU7JSI/89cgjjbApGSLuYdKS?=
 =?us-ascii?Q?eRsAGIZ5lgCagyltYQEvYtT+odVKzXcS1DwT0vPOw8CJLXO0iCRdRVtSrGaI?=
 =?us-ascii?Q?VdBD9kDm2fVhJxb3CiQ5Ixaa5x8DHCD6S3jR89bKfuUCMPEvwdcLdVkfIJuo?=
 =?us-ascii?Q?v85+PwU4zXjs3dgIHYBvzpeKtK5HbW3gfhPefAd9rohFAulOdXYFaFdWtZnn?=
 =?us-ascii?Q?h6Zf038h1fyCABv13qEpOGbhsTI76pwfef8Gbc9z4bsLEQSRNmbrU3Xq4s5A?=
 =?us-ascii?Q?TcddSzm2ggaixx1BNn+Lh6QYqJQ7EVIL+mgb4er4FO4c55OX3+L7InUm4n7z?=
 =?us-ascii?Q?Qc2f/qp0U0U0H7F0NbssRrKR1d2nye9BmPJvKy8y5OJwFYwek4qSTTxsdEiO?=
 =?us-ascii?Q?c6K88gmOh7HQGOs96sK2PE3AWpmtyOhvQYN4rO3ioBDOtWrzFXjuvz6HKCl0?=
 =?us-ascii?Q?1ZLRPPG+NK3G7CHcbDyzbic5/vG0q673chYXzgiJanzh9eFiHbBQHdbbwOH7?=
 =?us-ascii?Q?qIpw54NpLtFWPadOyZDqfaUwmJqKbtO1poOLsEjwfyeiRZjZ5AW+nKYNJ0uu?=
 =?us-ascii?Q?Wca6vm6YpyT//wz5VFdeCwSJlIU6XQL38RV+uHsdImrOQA3HsVjTl+bcWOXJ?=
 =?us-ascii?Q?sJy0WsWNUipHxgSXz18WGqhTOYnKaIBR9ktRgwd86xDfUjAFy2k8o3mjkdPc?=
 =?us-ascii?Q?lcPhpSh9hV4dbLPgTl2hy5OGPMk6syO2IyASPwhd4hHuwE5FVzP8uIGbXh0L?=
 =?us-ascii?Q?tmj72elQyChb97u5Enlq0oxmvL03XDEtrlBULwvkD69gCWXiZolitxq0zd4H?=
 =?us-ascii?Q?vqGH+PJ1H+SSvTTZ4THJD9Q6TMb4CH27bPJEQIN1NnKXKNu6N2knW0SoEG5U?=
 =?us-ascii?Q?N1/TprUFjbJgpapHGqhY7bgjDmng8Qn66NsCuNMzSeqGp0eEtT4nVw+eSOGf?=
 =?us-ascii?Q?AkT2qoKWG1Wsf2DmfLqFTI7DwKzj5dbs/eczuXMY9Hjmy+zLkXcuksFYfmCD?=
 =?us-ascii?Q?lWG1sRYQf3Iiy1fP+Cdd8TgtD13bHKAMDdYlkzvlwuaSsgWWNK80YEKIuYk9?=
 =?us-ascii?Q?bg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 62f2ab86-d7ab-4f9b-d0e2-08db555bd9db
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5471.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 May 2023 15:48:39.0948
 (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: 0R1wj/BfqgVm3Z8S3Zah03D5gFWznzifLYEWABTcewMUXRJUTyl1UHtyb9KCcgR/7rTqV3QLDQq1NDHKj91Uuw/uuRUBf1cYz063BPNdtr0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6480
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,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,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:07:19PM +0200, Jesper Dangaard Brouer wrote:
> 
> 
> On 15/05/2023 15.41, Larysa Zaremba wrote:
> > > > +	*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.
> > 
> 
> IIRC then VLAN tag zero is also a valid id, right?

AFAIK, 0x000 is reseved and basically means "no vlan tag". When ice hardware 
returns such value in descriptor, it says "no vlan tag was stripped" and this 
doesn't necessarily mean there is no VLAN tag in the packet.

For example, let us consider a packet:

  Ether/802.1ad(s-tag)/802.1q(c-tag)/...

Hardware does not strip c-tag in such case and sends 0x000 in the descriptor, 
but packet clearly does contain a c-tag, so at least in ice, it is reasonable to
not consider '0' a reliable value.

I guess, for s-tag value of 0x000 should be more reliable, so maybe
'if (!*vlan_tag)' usage can be limited to c-tag function.

> 
> --Jesper
>