From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2F475A04DD; Tue, 31 Dec 2019 03:13:50 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 368D21C036; Tue, 31 Dec 2019 03:13:49 +0100 (CET) Received: from mail-vs1-f67.google.com (mail-vs1-f67.google.com [209.85.217.67]) by dpdk.org (Postfix) with ESMTP id 567631C034 for ; Tue, 31 Dec 2019 03:13:47 +0100 (CET) Received: by mail-vs1-f67.google.com with SMTP id g23so21992640vsr.7 for ; Mon, 30 Dec 2019 18:13:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qOA95tKWtSeACzkZomHlAGJnjikQiwOmJNidz70QZus=; b=VJjt+fdZDutDYICqXnCydQ/HXTkBvN2aRF8C2vAo6VdBZn2/xpr1ewF2O5yKKNV/Kz B779eOy16bRzRct6TySuE7umq5M/ikV91elCT31p5gH176MLuEkyHicE3sEnYvFjTveD CufwWEfZp822UX0W7hDo+bhCvm8eVz6g2Oi/g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qOA95tKWtSeACzkZomHlAGJnjikQiwOmJNidz70QZus=; b=YwR6gcR+lslaH+Me4CK5ul2R7iSf7DToZJQJjQ8KoGgj72R1K/FOJTjNYoE3IsuDWV FShHmfws0RE0CBh6ECB4w/97ryAhWtWmWticPZ6bkKYZaRenJzmcgJTRu3FLyB3Mltgg 44NXm1NeHO6Dh3v1hVrxoAL+Xjypfq+UqIEF87xL+QbZabiWSqBbmcK9enmlrzMqSY0E STIaRInxtRrhqCGotw/QF3qz7UpOuUbkDx97agd4wJY3GyqKoBneUgVMrq+Ds6bTL4v9 QxRpNdlo/5kJjd25dL5YpudcZ5MW5ASeJOA033KhiWjdlvM6DHa4OmQXS7qJZH9zU2Oh q+iQ== X-Gm-Message-State: APjAAAUiru8ZZ3Nyb50APIEJq7fppaqE3JsdGCR2cx/A8eSekxVEu/N6 0bMZbvF28kzegN/AO0L2v+MBI49QUNtPlQJXseXpyw== X-Google-Smtp-Source: APXvYqwhdKEVhySslmtsmtoHuaiUzKeS5Udmt9z1lPoN1TOz9Yxuw9KtgzWTqnz3qZI94shOav8GTht3dO5inhpZT3E= X-Received: by 2002:a67:1884:: with SMTP id 126mr37710413vsy.219.1577758426449; Mon, 30 Dec 2019 18:13:46 -0800 (PST) MIME-Version: 1.0 References: <20191216031647.7750-1-somnath.kotur@broadcom.com> <68e1721c-4051-0451-185d-39344a026a38@solarflare.com> <53c0b3d2-5d61-6300-178e-b9500a93a7e3@solarflare.com> <4000b4f4-cdcd-78af-de60-2e8bde1364f7@solarflare.com> <20191227145041.GQ22738@platinum> In-Reply-To: <20191227145041.GQ22738@platinum> From: Somnath Kotur Date: Tue, 31 Dec 2019 07:43:33 +0530 Message-ID: To: Olivier Matz Cc: Andrew Rybchenko , dev , Ferruh Yigit , Hemant Agrawal , Sachin Saxena , Thomas Monjalon , David Marchand , JP Lee Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Dec 27, 2019 at 8:20 PM Olivier Matz wrote: > > Hi, > > On Tue, Dec 24, 2019 at 12:53:21PM +0300, Andrew Rybchenko wrote: > > On 12/24/19 6:16 AM, Somnath Kotur wrote: > > > Given that we haven't heard any objection from anyone in a while on > > > this ...can we get this in please? > > > > I'm sorry, but have you seen below? > > It means that PKT_RX_QINQ_STRIPPED, PKT_RX_QINQ, PKT_RX_VLAN > > and PKT_RX_VLAN_STRIPPED must be clarified. > > > > It sounds like change of semantics in order to resolve the > > problem, but anyway it is still a small change of semantics. > > Let's take this packet: > packet = ether | outer-vlan | inner-vlan | ... > > The possible mbufs received from a PMD, depending on configuration, are: > > 1/ no flag (no offload) > > 2/ PKT_RX_VLAN > packet data is unmodified > m->vlan_tci=outer-vlan > > 3/ PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED > outer-vlan is removed from packet data > m->vlan_tci=outer-vlan > > 4/ PKT_RX_VLAN | PKT_RX_QINQ > packet data is unmodified > m->vlan_tci_outer=outer-vlan > m->vlan_tci=inner-vlan > > 5/ PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED | PKT_RX_QINQ > > from PKT_RX_VLAN_STRIPPED: > A vlan has been stripped by the hardware and its tci is saved in > mbuf->vlan_tci. > from PKT_RX_QINQ: > The RX packet is a double VLAN, and the outer tci has been > saved in in mbuf->vlan_tci_outer. > Agree with your interpretation upto this point... > To me, it means: > > inner-vlan is removed from packet data > m->vlan_tci_outer=outer-vlan > m->vlan_tci=inner-vlan This is the case my patch wanted to cover ..There could be some HW which may not be able to strip and provide both VLANs in the QinQ scenario ... > > 6/ PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED | PKT_RX_QINQ | PKT_RX_QINQ_STRIPPED > both outer-vlan and inner-vlan removed from packet data > m->vlan_tci_outer=outer-vlan > m->vlan_tci=inner-vlan > > Other flag combinations are not supported. > The proposed patch documents that this new flag combination is now allowed: > > 7/ PKT_RX_VLAN | PKT_RX_QINQ | PKT_RX_QINQ_STRIPPED > outer-vlan is removed from packet data > m->vlan_tci_outer=outer-vlan > m->vlan_tci=inner-vlan > > Except if I missed something, I don't see any semantic change in > the previously supported cases. > > I think we should by the way clarify what happens in 5/, probably by > saying somewhere that as soon as PKT_RX_QINQ is set, PKT_RX_VLAN* > refer to inner vlan. > > > > BTW, it is better to make summary human readable and avoid > > PKT_RX_QINQ_STRIPPED (I guess check-git-log.sh yells on it). > > > > Also RFC patch cannot be applied, non-RFC version is required. > > > > CC main tree maintainers. > > > > > Thanks > > > > > > On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko > > > wrote: > > >> > > >> On 12/16/19 11:47 AM, Somnath Kotur wrote: > > >>> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko > > >>> wrote: > > >>>> > > >>>> On 12/16/19 6:16 AM, Somnath Kotur wrote: > > >>>>> Certain hardware may be able to strip and/or save only the outermost > > >>>>> VLAN instead of both the VLANs in the mbuf in a QinQ scenario. > > >>>>> To handle such cases, we could re-interpret setting of just PKT_RX_QINQ_STRIPPED > > >>>>> to indicate that only the outermost VLAN has been stripped by the hardware and > > >>>>> saved in mbuf->vlan_tci_outer. > > >>>>> Only When both PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set, the 2 vlans > > >>>>> have been stripped by the hardware and their tci are saved in mbuf->vlan_tci (inner) > > >>>>> and mbuf->vlan_tci_outer (outer). > > >>>>> > > >>>>> Signed-off-by: Somnath Kotur > > >>>>> Signed-off-by: JP Lee > > >>>>> --- > > >>>>> lib/librte_mbuf/rte_mbuf_core.h | 15 +++++++++++---- > > >>>>> 1 file changed, 11 insertions(+), 4 deletions(-) > > >>>>> > > >>>>> diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h > > >>>>> index 9a8557d..db1070b 100644 > > >>>>> --- a/lib/librte_mbuf/rte_mbuf_core.h > > >>>>> +++ b/lib/librte_mbuf/rte_mbuf_core.h > > >>>>> @@ -124,12 +124,19 @@ > > >>>>> #define PKT_RX_FDIR_FLX (1ULL << 14) > > >>>>> > > >>>>> /** > > >>>>> - * The 2 vlans have been stripped by the hardware and their tci are > > >>>>> - * saved in mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer). > > >>>>> + * The outer vlan has been stripped by the hardware and their tci are > > >>>>> + * saved in mbuf->vlan_tci_outer (outer). > > >>>>> * This can only happen if vlan stripping is enabled in the RX > > >>>>> * configuration of the PMD. > > >>>>> - * When PKT_RX_QINQ_STRIPPED is set, the flags (PKT_RX_VLAN | > > >>>>> - * PKT_RX_VLAN_STRIPPED | PKT_RX_QINQ) must also be set. > > >>>>> + * When PKT_RX_QINQ_STRIPPED is set, the flags (PKT_RX_VLAN | PKT_RX_QINQ) > > >>>>> + * must also be set. > > >>>>> + * When both PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set, the 2 vlans > > >>>>> + * have been stripped by the hardware and their tci are saved in > > >>>>> + * mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer). > > >>>>> + * This can only happen if vlan stripping is enabled in the RX configuration > > >>>>> + * of the PMD. > > >>>>> + * When PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set, > > >>>>> + * (PKT_RX_VLAN | PKT_RX_QINQ) must also be set. > > >>>>> */ > > >>>>> #define PKT_RX_QINQ_STRIPPED (1ULL << 15) > > >>>>> > > >>>> > > >>>> I always thought that PKT_RX_VLAN_STRIPPED means *one* VLAN > > >>>> stripped regardless if it is outer (if the packet is double > > >>>> tagged) or inner (if only one VLAN tag was present). > > >>>> > > >>>> That's why PKT_RX_QINQ_STRIPPED description says that *two* > > >>>> VLANs have been stripped. > > >>>> > > >>>> What is the problem with such approach? > > >>> The problem is that RX_VLAN_STRIPPED implies that the stripped VLAN > > >>> (outer or inner) is saved in mbuf->vlan_tci, correct? > > >> > > >> Yes. > > >> > > >>> There is no way to convey that it is in QinQ mode and yet only outer > > >>> VLAN has been stripped and saved in mbuf->vlan_tci_outer ? > > >> > > >> Ah, it looks like I understand now that the problem is in > > >> PKT_RX_QINQ description which claims that TCI is saved in > > >> mbuf->vlan_tci_outer and PKT_RX_QINQ_STRIPPED means that > > >> both VLAN tags are stripped regardless (PKT_RX_VLAN_STRIPPED). > > >> Moreover PKT_RX_QINQ_STRIPPED requires PKT_RX_VLAN_STRIPPED. > > >> > > >> It means that PKT_RX_QINQ_STRIPPED, PKT_RX_QINQ, PKT_RX_VLAN > > >> and PKT_RX_VLAN_STRIPPED must be clarified. > > >> > > >> I'm not sure, but it looks like it could affect net/dpaa2, > > >> so I'm including driver maintainers in CC. > >