diff mbox

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

Message ID f475d2d0fda01cf521578cda8af7019c15058d5e.1481590851.git.mirq-linux@rere.qmqm.pl (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel Per-patch compilation check success Compilation OK

Commit Message

Michał Mirosław Dec. 13, 2016, 1:08 a.m. UTC 
  Signed-off-by: Michał Mirosław <michal.miroslaw@atendesoftware.pl>

---
 lib/librte_net/rte_ether.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Thomas Monjalon Jan. 30, 2017, 9:54 a.m. UTC | #1 
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().
Olivier Matz Feb. 9, 2017, 3:56 p.m. UTC | #2 
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.

Regards,
Olivier
Thomas Monjalon April 30, 2017, 3:58 p.m. UTC | #3 
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?
Olivier Matz May 4, 2017, 7:30 a.m. UTC | #4 
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
diff mbox

Patch

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index ff3d065..26a8843 100644
--- 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 */