Re: [dpdk-dev] [PATCH 03/13] rte_ether: set PKT_RX_VLAN_STRIPPED in rte_vlan_strip()

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Thomas,

On Sun, 30 Apr 2017 17:58:45 +0200, Thomas Monjalon <thomas@monjalon.net> wrote:
> 09/02/2017 16:56, Olivier MATZ:
> > Hi,
> > 
> > On Mon, 30 Jan 2017 10:54:08 +0100, Thomas Monjalon
> > <thomas.monjalon@6wind.com> wrote:  
> > > It is fixing the introduction of the new flag PKT_RX_VLAN_STRIPPED.
> > > 
> > > Fixes: b37b528d957c ("mbuf: add new Rx flags for stripped VLAN")
> > > 
> > > This patch is applying the flag to the software emulation case
> > > (currently only for virtio).
> > > So the comment of this flag should be changed:
> > > 
> > > /**
> > >  * 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.
> > >  */
> > > #define PKT_RX_VLAN_STRIPPED (1ULL <<
> > > 6)                                                                         
> > > 
> > >   
> > > > Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>    
> > > [...]  
> > > > --- a/lib/librte_net/rte_ether.h
> > > > +++ b/lib/librte_net/rte_ether.h
> > > > @@ -357,7 +357,7 @@ static inline int rte_vlan_strip(struct
> > > > rte_mbuf *m) return -1;
> > > >  
> > > >  	struct vlan_hdr *vh = (struct vlan_hdr *)(eh + 1);
> > > > -	m->ol_flags |= PKT_RX_VLAN_PKT;
> > > > +	m->ol_flags |= PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED;
> > > >  	m->vlan_tci = rte_be_to_cpu_16(vh->vlan_tci);
> > > >  
> > > >  	/* Copy ether header over rather than moving whole packet
> > > > */    
> > > 
> > > I think this flag should also be removed in the function
> > > rte_vlan_insert().  
> > 
> > Agree with Thomas, I think rte_vlan_insert() should be updated too.
> > 
> > But I don't think the comment of the mbuf flag should be changed:
> > "stripped by the hardware" is a bit ambiguous for virtual drivers, but
> > we can understand that for virtual drivers the same work is done in
> > software.  
> 
> No more comment?
> 
> Olivier, the author is not replying.
> I think we should have updated the patch ourself.
> How risky it is for 17.05?
> Should it wait for 17.08?

I don't feel it's too risky for 17.05.
It's used in virtio and af_packet drivers, only when using vlan offload.

FYI, for 17.08, I plan to put the mbuf vlan flag subject on the table
again: when I introduced the new flag VLAN_STRIPPED, we acted that another
flag or pkt_type had to be introduced, but it was not really finished.

Olivier