From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [PATCH v2] mbuf: new flag when Vlan is stripped Date: Mon, 13 Jun 2016 18:07:00 +0200 Message-ID: <575EDA24.3020405@6wind.com> References: <1463993205-5623-1-git-send-email-olivier.matz@6wind.com> <1464359593-3534-1-git-send-email-olivier.matz@6wind.com> <2601191342CEEE43887BDE71AB97725836B70190@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: "johndale@cisco.com" , "Zhang, Helin" , "adrien.mazarguil@6wind.com" , "rahul.lakkireddy@chelsio.com" , "alejandro.lucero@netronome.com" , "sony.chacko@qlogic.com" To: "Ananyev, Konstantin" , "dev@dpdk.org" Return-path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id C79965938 for ; Mon, 13 Jun 2016 18:07:13 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB97725836B70190@irsmsx105.ger.corp.intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Konstantin, On 06/13/2016 04:42 PM, Ananyev, Konstantin wrote: >> The behavior of PKT_RX_VLAN_PKT was not very well defined, resulting in >> PMDs not advertising the same flags in similar conditions. >> >> Following discussion in [1], introduce 2 new flags PKT_RX_VLAN_STRIPPED >> and PKT_RX_QINQ_STRIPPED that are better defined: >> >> PKT_RX_VLAN_STRIPPED: a vlan has been stripped by the hardware and its >> tci is saved in mbuf->vlan_tci. This can only happen if vlan stripping >> is enabled in the RX configuration of the PMD. >> >> For now, the old flag PKT_RX_VLAN_PKT is kept but marked as deprecated. >> It should be removed from applications and PMDs in a future revision. > > I am not sure it has to be deprecated & removed. > ixgbe (and igb as I can read the specs) devices can provide information is that > a vlan packet or not even when vlan stripping is disabled. > Right now ixgbe PMD do carry thins information to the user, > and I suppose igb could be improved to carry it too. > So obviously we need a way to pass that information to the upper layer. > I remember it was a discussion about introducing new packet_type > instead of ol_flag value PKT_RX_VLAN_PKT. > But right now it is not there, and again I don't know how easy it would be to replace > one with another without performance considering that packet_type is not supported > now by ixgbe vRX. > If we would be able to replace it, then yes we can deprecate and drop the PKT_RX_VLAN_PKT. > But till then, I think we'd better keep it. I think the packet_type feature would be more appropriate than a flag for carrying this kind of info. Currently the behavior of PKT_RX_VLAN_PKT is not properly defined, and it is not the same on all PMDs. So, from an application perspective, it's not usable except if it knows that the underlying PMD is an ixgbe. This is not acceptable for a generic API and that's why I think this flag, as it is today, should be deprecated. It won't prevent an application from using the flag right after my commit, but it will warn the user that the flag should not be used as is. If someone is willing to work on this feature for 16.11, why not but again, I think using the packet_type is more appropriate. The problem is that I don't want to have this flag in this state forever, and I also don't want to add in rte_mbuf.h a comment saying "this flag does this on ixgbe and that on other drivers". If we decide to generalize the ixgbe behavior for all PMDs for this flag, it will break the applications relying on this flag but with other PMDs. So for the same reason we added a new PKT_RX_VLAN_STRIPPED we cannot change the behavior of an existing flag. >> @@ -270,6 +277,11 @@ _recv_raw_pkts_vec(struct ixgbe_rx_queue *rxq, struct rte_mbuf **rx_pkts, >> */ >> sw_ring = &rxq->sw_ring[rxq->rx_tail]; >> >> + /* ensure these 2 flags are in the lower 8 bits */ >> + RTE_BUILD_BUG_ON(((PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED) & >> + 0xffffffffffffff00ULL) != 0ULL); > > > I suppose your need here: > RTE_BUILD_BUG_ON(((PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED) & UINT8_MAX) == > PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED)); > > To make sure both flags are inside first 8 bits? Yes, indeed that's much better than 14 'f' :) >> + vlan_flags = rxq->vlan_flags & 0xff; >> + > > Probably better to do that check/ AND at setup phase, not run-time? > As a nit: s/0xff/UINT8_MAX/. Yep, agree, I'll change that. Thanks for reviewing Olivier