diff mbox

[dpdk-dev,2/5] mbuf: use the reserved 16 bits for double vlan

Message ID 1432629400-25303-3-git-send-email-helin.zhang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Zhang, Helin May 26, 2015, 8:36 a.m. UTC 
  Use the reserved 16 bits in rte_mbuf structure for the outer vlan,
also add QinQ offloading flags for both RX and TX sides.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 lib/librte_mbuf/rte_mbuf.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Stephen Hemminger May 26, 2015, 2:55 p.m. UTC | #1 
On Tue, 26 May 2015 16:36:37 +0800
Helin Zhang <helin.zhang@intel.com> wrote:

> Use the reserved 16 bits in rte_mbuf structure for the outer vlan,
> also add QinQ offloading flags for both RX and TX sides.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>

Yet another change that is much needed, but breaks ABI compatibility.
Zhang, Helin May 26, 2015, 3 p.m. UTC | #2 
Hi Stephen

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, May 26, 2015 10:55 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits for
> double vlan
> 
> On Tue, 26 May 2015 16:36:37 +0800
> Helin Zhang <helin.zhang@intel.com> wrote:
> 
> > Use the reserved 16 bits in rte_mbuf structure for the outer vlan,
> > also add QinQ offloading flags for both RX and TX sides.
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> 
> Yet another change that is much needed, but breaks ABI compatibility.
Even just use the reserved 16 bits? It seems yes.
Would it be acceptable to use the original name of 'reserved' for the outer vlan?
And then announce the name change, and rename it one release after?

Regards,
Helin
Ananyev, Konstantin May 26, 2015, 3:02 p.m. UTC | #3 
Hi Stephen,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Tuesday, May 26, 2015 3:55 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits for double vlan
> 
> On Tue, 26 May 2015 16:36:37 +0800
> Helin Zhang <helin.zhang@intel.com> wrote:
> 
> > Use the reserved 16 bits in rte_mbuf structure for the outer vlan,
> > also add QinQ offloading flags for both RX and TX sides.
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> 
> Yet another change that is much needed, but breaks ABI compatibility.

Why do you think it breaks ABI compatibility?
As I can see, it uses field that was reserved.
Konstantin
Stephen Hemminger May 26, 2015, 3:35 p.m. UTC | #4 
On Tue, 26 May 2015 15:02:51 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:

> Hi Stephen,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Tuesday, May 26, 2015 3:55 PM
> > To: Zhang, Helin
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits for double vlan
> > 
> > On Tue, 26 May 2015 16:36:37 +0800
> > Helin Zhang <helin.zhang@intel.com> wrote:
> > 
> > > Use the reserved 16 bits in rte_mbuf structure for the outer vlan,
> > > also add QinQ offloading flags for both RX and TX sides.
> > >
> > > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > 
> > Yet another change that is much needed, but breaks ABI compatibility.
> 
> Why do you think it breaks ABI compatibility?
> As I can see, it uses field that was reserved.
> Konstantin

Because an application maybe assuming something or reusing the reserved fields.
Yes, it would be dumb of application to do that but from absolute ABI point
of view it is a change.
Ananyev, Konstantin May 26, 2015, 3:46 p.m. UTC | #5 
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, May 26, 2015 4:35 PM
> To: Ananyev, Konstantin
> Cc: Zhang, Helin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits for double vlan
> 
> On Tue, 26 May 2015 15:02:51 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> 
> > Hi Stephen,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > Sent: Tuesday, May 26, 2015 3:55 PM
> > > To: Zhang, Helin
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits for double vlan
> > >
> > > On Tue, 26 May 2015 16:36:37 +0800
> > > Helin Zhang <helin.zhang@intel.com> wrote:
> > >
> > > > Use the reserved 16 bits in rte_mbuf structure for the outer vlan,
> > > > also add QinQ offloading flags for both RX and TX sides.
> > > >
> > > > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > >
> > > Yet another change that is much needed, but breaks ABI compatibility.
> >
> > Why do you think it breaks ABI compatibility?
> > As I can see, it uses field that was reserved.
> > Konstantin
> 
> Because an application maybe assuming something or reusing the reserved fields.

But properly behaving application, shouldn't do that right?
And for misbehaving ones, why should we care about them?

> Yes, it would be dumb of application to do that but from absolute ABI point
> of view it is a change.

So, in theory,  even adding a new field to the end of rte_mbuf is an ABI breakage?
Konstantin
Zhang, Helin May 27, 2015, 1:07 a.m. UTC | #6 
> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, May 26, 2015 11:46 PM
> To: Stephen Hemminger
> Cc: Zhang, Helin; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits for
> double vlan
> 
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, May 26, 2015 4:35 PM
> > To: Ananyev, Konstantin
> > Cc: Zhang, Helin; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits for
> > double vlan
> >
> > On Tue, 26 May 2015 15:02:51 +0000
> > "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> >
> > > Hi Stephen,
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > > > Hemminger
> > > > Sent: Tuesday, May 26, 2015 3:55 PM
> > > > To: Zhang, Helin
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits
> > > > for double vlan
> > > >
> > > > On Tue, 26 May 2015 16:36:37 +0800 Helin Zhang
> > > > <helin.zhang@intel.com> wrote:
> > > >
> > > > > Use the reserved 16 bits in rte_mbuf structure for the outer
> > > > > vlan, also add QinQ offloading flags for both RX and TX sides.
> > > > >
> > > > > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > > >
> > > > Yet another change that is much needed, but breaks ABI
> compatibility.
> > >
> > > Why do you think it breaks ABI compatibility?
> > > As I can see, it uses field that was reserved.
> > > Konstantin
> >
> > Because an application maybe assuming something or reusing the
> reserved fields.
> 
> But properly behaving application, shouldn't do that right?
> And for misbehaving ones, why should we care about them?
For any reserved bits, I think all application users should avoid touching it,
as it is reserved for future use, or some special reason. Otherwise,
un-predicted behavior can be expected.

Regards,
Helin

> 
> > Yes, it would be dumb of application to do that but from absolute ABI
> > point of view it is a change.
> 
> So, in theory,  even adding a new field to the end of rte_mbuf is an ABI
> breakage?
> Konstantin
Olivier Matz June 1, 2015, 8:50 a.m. UTC | #7 
Hi Helin,

On 05/26/2015 10:36 AM, Helin Zhang wrote:
> Use the reserved 16 bits in rte_mbuf structure for the outer vlan,
> also add QinQ offloading flags for both RX and TX sides.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  lib/librte_mbuf/rte_mbuf.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index ab6de67..4551df9 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -101,11 +101,17 @@ extern "C" {
>  #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet with IPv6 header. */
>  #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR match. */
>  #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported if FDIR match. */
> +#define PKT_RX_QINQ_PKT     (1ULL << 15)  /**< RX packet with double VLAN stripped. */
>  /* add new RX flags here */

There's a small indent typo here: (1ULL << 15) is not aligned
with the lines above


>  
>  /* add new TX flags here */
>  
>  /**
> + * Second VLAN insertion (QinQ) flag.
> + */
> +#define PKT_TX_QINQ_PKT    (1ULL << 49)   /**< TX packet with double VLAN inserted. */
> +
> +/**
>   * TCP segmentation offload. To enable this offload feature for a
>   * packet to be transmitted on hardware supporting TSO:
>   *  - set the PKT_TX_TCP_SEG flag in mbuf->ol_flags (this flag implies
> @@ -279,7 +285,7 @@ struct rte_mbuf {
>  	uint16_t data_len;        /**< Amount of data in segment buffer. */
>  	uint32_t pkt_len;         /**< Total pkt len: sum of all segments. */
>  	uint16_t vlan_tci;        /**< VLAN Tag Control Identifier (CPU order) */
> -	uint16_t reserved;
> +	uint16_t vlan_tci_outer;  /**< Outer VLAN Tag Control Identifier (CPU order) */
>  	union {
>  		uint32_t rss;     /**< RSS hash result if RSS enabled */
>  		struct {
> @@ -777,6 +783,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
>  	m->pkt_len = 0;
>  	m->tx_offload = 0;
>  	m->vlan_tci = 0;
> +	m->vlan_tci_outer = 0;
>  	m->nb_segs = 1;
>  	m->port = 0xff;
>  
> @@ -849,6 +856,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>  	mi->data_len = m->data_len;
>  	mi->port = m->port;
>  	mi->vlan_tci = m->vlan_tci;
> +	mi->vlan_tci_outer = m->vlan_tci_outer;
>  	mi->tx_offload = m->tx_offload;
>  	mi->hash = m->hash;
>  
> 

Maybe some more affectations are missing. For instance in
examples/ipv4_multicast/main.c or in examples/vhost/main.c.
You can grep "->vlan_tci =" to find them all.

Do we need to update rte_vlan_insert() and rte_vlan_strip() to
support QinQ?

Regards,
Olivier
Zhang, Helin June 2, 2015, 2:37 a.m. UTC | #8 
> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Monday, June 1, 2015 4:50 PM
> To: Zhang, Helin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/5] mbuf: use the reserved 16 bits for double
> vlan
> 
> Hi Helin,
> 
> On 05/26/2015 10:36 AM, Helin Zhang wrote:
> > Use the reserved 16 bits in rte_mbuf structure for the outer vlan,
> > also add QinQ offloading flags for both RX and TX sides.
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index ab6de67..4551df9 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -101,11 +101,17 @@ extern "C" {
> >  #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet with
> IPv6 header. */
> >  #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR
> match. */
> >  #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported if
> FDIR match. */
> > +#define PKT_RX_QINQ_PKT     (1ULL << 15)  /**< RX packet with double
> VLAN stripped. */
> >  /* add new RX flags here */
> 
> There's a small indent typo here: (1ULL << 15) is not aligned with the lines above
Will fix it.

> 
> 
> >
> >  /* add new TX flags here */
> >
> >  /**
> > + * Second VLAN insertion (QinQ) flag.
> > + */
> > +#define PKT_TX_QINQ_PKT    (1ULL << 49)   /**< TX packet with double
> VLAN inserted. */
> > +
> > +/**
> >   * TCP segmentation offload. To enable this offload feature for a
> >   * packet to be transmitted on hardware supporting TSO:
> >   *  - set the PKT_TX_TCP_SEG flag in mbuf->ol_flags (this flag
> > implies @@ -279,7 +285,7 @@ struct rte_mbuf {
> >  	uint16_t data_len;        /**< Amount of data in segment buffer. */
> >  	uint32_t pkt_len;         /**< Total pkt len: sum of all segments. */
> >  	uint16_t vlan_tci;        /**< VLAN Tag Control Identifier (CPU order) */
> > -	uint16_t reserved;
> > +	uint16_t vlan_tci_outer;  /**< Outer VLAN Tag Control Identifier
> > +(CPU order) */
> >  	union {
> >  		uint32_t rss;     /**< RSS hash result if RSS enabled */
> >  		struct {
> > @@ -777,6 +783,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf
> *m)
> >  	m->pkt_len = 0;
> >  	m->tx_offload = 0;
> >  	m->vlan_tci = 0;
> > +	m->vlan_tci_outer = 0;
> >  	m->nb_segs = 1;
> >  	m->port = 0xff;
> >
> > @@ -849,6 +856,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf
> *mi, struct rte_mbuf *m)
> >  	mi->data_len = m->data_len;
> >  	mi->port = m->port;
> >  	mi->vlan_tci = m->vlan_tci;
> > +	mi->vlan_tci_outer = m->vlan_tci_outer;
> >  	mi->tx_offload = m->tx_offload;
> >  	mi->hash = m->hash;
> >
> >
> 
> Maybe some more affectations are missing. For instance in
> examples/ipv4_multicast/main.c or in examples/vhost/main.c.
> You can grep "->vlan_tci =" to find them all.
Will add vlan_tci_outer in ipv4_multicast/main.c.
After talking with vhost developers, it does not need to support double vlan at
this moment, so I will keep it as is.

> 
> Do we need to update rte_vlan_insert() and rte_vlan_strip() to support QinQ?
They are the software version of vlan stripping and insertion. It was mainly for virtio.
I'd like to keep it as is, and let who want it to develop the double vlan stripping/insertion
version in the future.

Thank you very much!
- Helin

> 
> Regards,
> Olivier
diff mbox

Patch

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index ab6de67..4551df9 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -101,11 +101,17 @@  extern "C" {
 #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet with IPv6 header. */
 #define PKT_RX_FDIR_ID       (1ULL << 13) /**< FD id reported if FDIR match. */
 #define PKT_RX_FDIR_FLX      (1ULL << 14) /**< Flexible bytes reported if FDIR match. */
+#define PKT_RX_QINQ_PKT     (1ULL << 15)  /**< RX packet with double VLAN stripped. */
 /* add new RX flags here */
 
 /* add new TX flags here */
 
 /**
+ * Second VLAN insertion (QinQ) flag.
+ */
+#define PKT_TX_QINQ_PKT    (1ULL << 49)   /**< TX packet with double VLAN inserted. */
+
+/**
  * TCP segmentation offload. To enable this offload feature for a
  * packet to be transmitted on hardware supporting TSO:
  *  - set the PKT_TX_TCP_SEG flag in mbuf->ol_flags (this flag implies
@@ -279,7 +285,7 @@  struct rte_mbuf {
 	uint16_t data_len;        /**< Amount of data in segment buffer. */
 	uint32_t pkt_len;         /**< Total pkt len: sum of all segments. */
 	uint16_t vlan_tci;        /**< VLAN Tag Control Identifier (CPU order) */
-	uint16_t reserved;
+	uint16_t vlan_tci_outer;  /**< Outer VLAN Tag Control Identifier (CPU order) */
 	union {
 		uint32_t rss;     /**< RSS hash result if RSS enabled */
 		struct {
@@ -777,6 +783,7 @@  static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
 	m->pkt_len = 0;
 	m->tx_offload = 0;
 	m->vlan_tci = 0;
+	m->vlan_tci_outer = 0;
 	m->nb_segs = 1;
 	m->port = 0xff;
 
@@ -849,6 +856,7 @@  static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
 	mi->data_len = m->data_len;
 	mi->port = m->port;
 	mi->vlan_tci = m->vlan_tci;
+	mi->vlan_tci_outer = m->vlan_tci_outer;
 	mi->tx_offload = m->tx_offload;
 	mi->hash = m->hash;