diff mbox

[dpdk-dev] librte_lpm: define tbl entry reversely for big endian

Message ID 1425450852-24837-1-git-send-email-xuelin.shi@freescale.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

xuelin.shi@freescale.com March 4, 2015, 6:34 a.m. UTC 
  From: Xuelin Shi <xuelin.shi@freescale.com>

This module uses type conversion between struct and int.
Also truncation and comparison is used with this int.
It is not safe for different endian arch.

Add ifdef for big endian struct to fix this issue.

Signed-off-by: Xuelin Shi <xuelin.shi@freescale.com>
---
 lib/librte_lpm/rte_lpm.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Bruce Richardson March 4, 2015, 10:48 a.m. UTC | #1 
On Wed, Mar 04, 2015 at 02:34:12PM +0800, xuelin.shi@freescale.com wrote:
> From: Xuelin Shi <xuelin.shi@freescale.com>
> 
> This module uses type conversion between struct and int.
> Also truncation and comparison is used with this int.
> It is not safe for different endian arch.
> 
> Add ifdef for big endian struct to fix this issue.
> 
> Signed-off-by: Xuelin Shi <xuelin.shi@freescale.com>
> ---
>  lib/librte_lpm/rte_lpm.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> index 1af150c..08a2859 100644
> --- a/lib/librte_lpm/rte_lpm.h
> +++ b/lib/librte_lpm/rte_lpm.h
> @@ -96,6 +96,7 @@ extern "C" {
>  /** Bitmask used to indicate successful lookup */
>  #define RTE_LPM_LOOKUP_SUCCESS          0x0100
>  
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>  /** @internal Tbl24 entry structure. */
>  struct rte_lpm_tbl24_entry {
>  	/* Stores Next hop or group index (i.e. gindex)into tbl8. */
> @@ -117,6 +118,24 @@ struct rte_lpm_tbl8_entry {
>  	uint8_t valid_group :1; /**< Group validation flag. */
>  	uint8_t depth       :6; /**< Rule depth. */
>  };
> +#else
> +struct rte_lpm_tbl24_entry {
> +	uint8_t depth 	    :6;
> +	uint8_t ext_entry   :1;
> +	uint8_t valid	    :1;

Since endianness only refers to the order of bytes within a word, do the bitfields
within the uint8_t really need to be swapped around too?

/Bruce

> +	union {
> +		uint8_t tbl8_gindex;
> +		uint8_t next_hop;
> +	};
> +};
> +
> +struct rte_lpm_tbl8_entry {
> +	uint8_t depth 	    :6;
> +	uint8_t valid_group :1;
> +	uint8_t valid	    :1;
> +	uint8_t next_hop;
> +};
> +#endif
>  
>  /** @internal Rule structure. */
>  struct rte_lpm_rule {
> -- 
> 1.9.1
>
xuelin.shi@freescale.com March 5, 2015, 2:12 a.m. UTC | #2 
Hi Bruce,

Yes, it needs to swap the fields. The bit field is first identified as the uint8_t and then packed.

Thanks,
Shi xuelin

> -----Original Message-----
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, March 04, 2015 18:48
> To: Shi Xuelin-B29237
> Cc: thomas.monjalon@6wind.com; dev@dpdk.org
> Subject: Re: [PATCH] librte_lpm: define tbl entry reversely for big
> endian
> 
> On Wed, Mar 04, 2015 at 02:34:12PM +0800, xuelin.shi@freescale.com wrote:
> > From: Xuelin Shi <xuelin.shi@freescale.com>
> >
> > This module uses type conversion between struct and int.
> > Also truncation and comparison is used with this int.
> > It is not safe for different endian arch.
> >
> > Add ifdef for big endian struct to fix this issue.
> >
> > Signed-off-by: Xuelin Shi <xuelin.shi@freescale.com>
> > ---
> >  lib/librte_lpm/rte_lpm.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h index
> > 1af150c..08a2859 100644
> > --- a/lib/librte_lpm/rte_lpm.h
> > +++ b/lib/librte_lpm/rte_lpm.h
> > @@ -96,6 +96,7 @@ extern "C" {
> >  /** Bitmask used to indicate successful lookup */
> >  #define RTE_LPM_LOOKUP_SUCCESS          0x0100
> >
> > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> >  /** @internal Tbl24 entry structure. */  struct rte_lpm_tbl24_entry {
> >  	/* Stores Next hop or group index (i.e. gindex)into tbl8. */ @@
> > -117,6 +118,24 @@ struct rte_lpm_tbl8_entry {
> >  	uint8_t valid_group :1; /**< Group validation flag. */
> >  	uint8_t depth       :6; /**< Rule depth. */
> >  };
> > +#else
> > +struct rte_lpm_tbl24_entry {
> > +	uint8_t depth 	    :6;
> > +	uint8_t ext_entry   :1;
> > +	uint8_t valid	    :1;
> 
> Since endianness only refers to the order of bytes within a word, do the
> bitfields within the uint8_t really need to be swapped around too?
> 
> /Bruce

> 
> > +	union {
> > +		uint8_t tbl8_gindex;
> > +		uint8_t next_hop;
> > +	};
> > +};
> > +
> > +struct rte_lpm_tbl8_entry {
> > +	uint8_t depth 	    :6;
> > +	uint8_t valid_group :1;
> > +	uint8_t valid	    :1;
> > +	uint8_t next_hop;
> > +};
> > +#endif
> >
> >  /** @internal Rule structure. */
> >  struct rte_lpm_rule {
> > --
> > 1.9.1
> >
Bruce Richardson March 6, 2015, 11:13 a.m. UTC | #3 
On Thu, Mar 05, 2015 at 02:12:12AM +0000, Xuelin Shi wrote:
> Hi Bruce,
> 
> Yes, it needs to swap the fields. The bit field is first identified as the uint8_t and then packed.
> 
> Thanks,
> Shi xuelin
> 
Am I right in thinking that this patch set supercedes that for
"lpm: use field access instead of type conversion" http://dpdk.org/dev/patchwork/patch/3132/ ?

> > -----Original Message-----
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Wednesday, March 04, 2015 18:48
> > To: Shi Xuelin-B29237
> > Cc: thomas.monjalon@6wind.com; dev@dpdk.org
> > Subject: Re: [PATCH] librte_lpm: define tbl entry reversely for big
> > endian
> > 
> > On Wed, Mar 04, 2015 at 02:34:12PM +0800, xuelin.shi@freescale.com wrote:
> > > From: Xuelin Shi <xuelin.shi@freescale.com>
> > >
> > > This module uses type conversion between struct and int.
> > > Also truncation and comparison is used with this int.
> > > It is not safe for different endian arch.
> > >
> > > Add ifdef for big endian struct to fix this issue.
> > >
> > > Signed-off-by: Xuelin Shi <xuelin.shi@freescale.com>
> > > ---
> > >  lib/librte_lpm/rte_lpm.h | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h index
> > > 1af150c..08a2859 100644
> > > --- a/lib/librte_lpm/rte_lpm.h
> > > +++ b/lib/librte_lpm/rte_lpm.h
> > > @@ -96,6 +96,7 @@ extern "C" {
> > >  /** Bitmask used to indicate successful lookup */
> > >  #define RTE_LPM_LOOKUP_SUCCESS          0x0100
> > >
> > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > >  /** @internal Tbl24 entry structure. */  struct rte_lpm_tbl24_entry {
> > >  	/* Stores Next hop or group index (i.e. gindex)into tbl8. */ @@
> > > -117,6 +118,24 @@ struct rte_lpm_tbl8_entry {
> > >  	uint8_t valid_group :1; /**< Group validation flag. */
> > >  	uint8_t depth       :6; /**< Rule depth. */
> > >  };
> > > +#else
> > > +struct rte_lpm_tbl24_entry {
> > > +	uint8_t depth 	    :6;
> > > +	uint8_t ext_entry   :1;
> > > +	uint8_t valid	    :1;
> > 
> > Since endianness only refers to the order of bytes within a word, do the
> > bitfields within the uint8_t really need to be swapped around too?
> > 
> > /Bruce
> 
> > 
> > > +	union {
> > > +		uint8_t tbl8_gindex;
> > > +		uint8_t next_hop;
> > > +	};
> > > +};
> > > +
> > > +struct rte_lpm_tbl8_entry {
> > > +	uint8_t depth 	    :6;
> > > +	uint8_t valid_group :1;
> > > +	uint8_t valid	    :1;
> > > +	uint8_t next_hop;
> > > +};
> > > +#endif
> > >
> > >  /** @internal Rule structure. */
> > >  struct rte_lpm_rule {
> > > --
> > > 1.9.1
> > >
xuelin.shi@freescale.com March 9, 2015, 1:54 a.m. UTC | #4 
Hi Bruce,

Yes, the patch http://dpdk.org/dev/patchwork/patch/3132/ should be abandoned.

Thanks,
Shi xuelin

> -----Original Message-----
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, March 06, 2015 19:14
> To: Shi Xuelin-B29237
> Cc: thomas.monjalon@6wind.com; dev@dpdk.org
> Subject: Re: [PATCH] librte_lpm: define tbl entry reversely for big
> endian
> 
> On Thu, Mar 05, 2015 at 02:12:12AM +0000, Xuelin Shi wrote:
> > Hi Bruce,
> >
> > Yes, it needs to swap the fields. The bit field is first identified as
> the uint8_t and then packed.
> >
> > Thanks,
> > Shi xuelin
> >
> Am I right in thinking that this patch set supercedes that for
> "lpm: use field access instead of type conversion"
> http://dpdk.org/dev/patchwork/patch/3132/ ?
> 
> > > -----Original Message-----
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Wednesday, March 04, 2015 18:48
> > > To: Shi Xuelin-B29237
> > > Cc: thomas.monjalon@6wind.com; dev@dpdk.org
> > > Subject: Re: [PATCH] librte_lpm: define tbl entry reversely for big
> > > endian
> > >
> > > On Wed, Mar 04, 2015 at 02:34:12PM +0800, xuelin.shi@freescale.com
> wrote:
> > > > From: Xuelin Shi <xuelin.shi@freescale.com>
> > > >
> > > > This module uses type conversion between struct and int.
> > > > Also truncation and comparison is used with this int.
> > > > It is not safe for different endian arch.
> > > >
> > > > Add ifdef for big endian struct to fix this issue.
> > > >
> > > > Signed-off-by: Xuelin Shi <xuelin.shi@freescale.com>
> > > > ---
> > > >  lib/librte_lpm/rte_lpm.h | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> > > > index
> > > > 1af150c..08a2859 100644
> > > > --- a/lib/librte_lpm/rte_lpm.h
> > > > +++ b/lib/librte_lpm/rte_lpm.h
> > > > @@ -96,6 +96,7 @@ extern "C" {
> > > >  /** Bitmask used to indicate successful lookup */
> > > >  #define RTE_LPM_LOOKUP_SUCCESS          0x0100
> > > >
> > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > >  /** @internal Tbl24 entry structure. */  struct
> rte_lpm_tbl24_entry {
> > > >  	/* Stores Next hop or group index (i.e. gindex)into tbl8. */
> @@
> > > > -117,6 +118,24 @@ struct rte_lpm_tbl8_entry {
> > > >  	uint8_t valid_group :1; /**< Group validation flag. */
> > > >  	uint8_t depth       :6; /**< Rule depth. */
> > > >  };
> > > > +#else
> > > > +struct rte_lpm_tbl24_entry {
> > > > +	uint8_t depth 	    :6;
> > > > +	uint8_t ext_entry   :1;
> > > > +	uint8_t valid	    :1;
> > >
> > > Since endianness only refers to the order of bytes within a word, do
> > > the bitfields within the uint8_t really need to be swapped around too?
> > >
> > > /Bruce
> >
> > >
> > > > +	union {
> > > > +		uint8_t tbl8_gindex;
> > > > +		uint8_t next_hop;
> > > > +	};
> > > > +};
> > > > +
> > > > +struct rte_lpm_tbl8_entry {
> > > > +	uint8_t depth 	    :6;
> > > > +	uint8_t valid_group :1;
> > > > +	uint8_t valid	    :1;
> > > > +	uint8_t next_hop;
> > > > +};
> > > > +#endif
> > > >
> > > >  /** @internal Rule structure. */
> > > >  struct rte_lpm_rule {
> > > > --
> > > > 1.9.1
> > > >
John McNamara March 9, 2015, 9:12 a.m. UTC | #5 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xuelin Shi
> Sent: Monday, March 9, 2015 1:54 AM
> To: Richardson, Bruce
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] librte_lpm: define tbl entry reversely for
> big endian
>
> Yes, the patch http://dpdk.org/dev/patchwork/patch/3132/ should be
> abandoned.
> 

Hi,

If you register and login to the DPDK patchwork site you can mark the patch as "Not Applicable" or whatever category now applies.

    http://dpdk.org/dev/patchwork/user/login/

John
xuelin.shi@freescale.com March 9, 2015, 9:35 a.m. UTC | #6 
Hi,

OK, done. Marked as "not applicable".

Thanks,
Shi 

> -----Original Message-----
> From: Mcnamara, John [mailto:john.mcnamara@intel.com]
> Sent: Monday, March 09, 2015 17:13
> To: Shi Xuelin-B29237; Richardson, Bruce
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] librte_lpm: define tbl entry reversely
> for big endian
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xuelin Shi
> > Sent: Monday, March 9, 2015 1:54 AM
> > To: Richardson, Bruce
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] librte_lpm: define tbl entry reversely
> > for big endian
> >
> > Yes, the patch http://dpdk.org/dev/patchwork/patch/3132/ should be
> > abandoned.
> >
> 
> Hi,
> 
> If you register and login to the DPDK patchwork site you can mark the
> patch as "Not Applicable" or whatever category now applies.
> 
>     http://dpdk.org/dev/patchwork/user/login/
> 
> John
Bruce Richardson March 9, 2015, 2:02 p.m. UTC | #7 
On Wed, Mar 04, 2015 at 02:34:12PM +0800, xuelin.shi@freescale.com wrote:
> From: Xuelin Shi <xuelin.shi@freescale.com>
> 
> This module uses type conversion between struct and int.
> Also truncation and comparison is used with this int.
> It is not safe for different endian arch.
> 
> Add ifdef for big endian struct to fix this issue.
> 
> Signed-off-by: Xuelin Shi <xuelin.shi@freescale.com>
> ---
>  lib/librte_lpm/rte_lpm.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> index 1af150c..08a2859 100644
> --- a/lib/librte_lpm/rte_lpm.h
> +++ b/lib/librte_lpm/rte_lpm.h
> @@ -96,6 +96,7 @@ extern "C" {
>  /** Bitmask used to indicate successful lookup */
>  #define RTE_LPM_LOOKUP_SUCCESS          0x0100
>  
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>  /** @internal Tbl24 entry structure. */
>  struct rte_lpm_tbl24_entry {
>  	/* Stores Next hop or group index (i.e. gindex)into tbl8. */
> @@ -117,6 +118,24 @@ struct rte_lpm_tbl8_entry {
>  	uint8_t valid_group :1; /**< Group validation flag. */
>  	uint8_t depth       :6; /**< Rule depth. */
>  };
> +#else
> +struct rte_lpm_tbl24_entry {
> +	uint8_t depth 	    :6;
> +	uint8_t ext_entry   :1;
> +	uint8_t valid	    :1;
> +	union {
> +		uint8_t tbl8_gindex;
> +		uint8_t next_hop;
> +	};
> +};
> +
> +struct rte_lpm_tbl8_entry {
> +	uint8_t depth 	    :6;
> +	uint8_t valid_group :1;
> +	uint8_t valid	    :1;
> +	uint8_t next_hop;
> +};
> +#endif
>  
>  /** @internal Rule structure. */
>  struct rte_lpm_rule {
> -- 
> 1.9.1
> 

Get an error compiling this up (using clang on FreeBSD).

  CC rte_lpm.o
  In file included from /usr/home/bruce/dpdk.org/lib/librte_lpm/rte_lpm.c:57:
  /usr/home/bruce/dpdk.org/lib/librte_lpm/rte_lpm.h:99:5: fatal error: 'RTE_BYTE_ORDER' is not defined, evaluates to 0 [-Wundef]
#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
      ^
      1 error generated.

Adding "#include <rte_byteorder.h>" should fix the issue.

Existing unit tests on IA (little endian) pass fine there-after, but I think for
this patch it would be good to have an ack from someone who can validate on
a big endian system, since this is what this patch is meant to enable.

/Bruce
Thomas Monjalon March 9, 2015, 2:04 p.m. UTC | #8 
2015-03-09 09:12, Mcnamara, John:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xuelin Shi
> > Yes, the patch http://dpdk.org/dev/patchwork/patch/3132/ should be
> > abandoned.
> 
> If you register and login to the DPDK patchwork site you can mark the patch
> as "Not Applicable" or whatever category now applies.
> 
>     http://dpdk.org/dev/patchwork/user/login/

"superseded" would be more appropriate here.
Generally, "not applicable" should be used for emails which are not real
patches or really wrongly formatted.
Thomas Monjalon March 23, 2015, 2:03 p.m. UTC | #9 
2015-03-09 14:02, Bruce Richardson:
> On Wed, Mar 04, 2015 at 02:34:12PM +0800, xuelin.shi@freescale.com wrote:
> > From: Xuelin Shi <xuelin.shi@freescale.com>
> > 
> > This module uses type conversion between struct and int.
> > Also truncation and comparison is used with this int.
> > It is not safe for different endian arch.
> > 
> > Add ifdef for big endian struct to fix this issue.
> > 
> > Signed-off-by: Xuelin Shi <xuelin.shi@freescale.com>
> 
> Get an error compiling this up (using clang on FreeBSD).
> 
>   CC rte_lpm.o
>   In file included from /usr/home/bruce/dpdk.org/lib/librte_lpm/rte_lpm.c:57:
>   /usr/home/bruce/dpdk.org/lib/librte_lpm/rte_lpm.h:99:5: fatal error: 'RTE_BYTE_ORDER' is not defined, evaluates to 0 [-Wundef]
> #if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>       ^
>       1 error generated.
> 
> Adding "#include <rte_byteorder.h>" should fix the issue.

Please Xuelin, could you submit a v2?
Thanks

> Existing unit tests on IA (little endian) pass fine there-after, but I think for
> this patch it would be good to have an ack from someone who can validate on
> a big endian system, since this is what this patch is meant to enable.
> 
> /Bruce
>
xuelin.shi@freescale.com March 24, 2015, 7:28 a.m. UTC | #10 
Hi Thomas,

Done.  http://patchwork.dpdk.org/dev/patchwork/patch/4122/

Thanks,
Xuelin Shi

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, March 23, 2015 22:04
> To: Shi Xuelin-B29237
> Cc: Bruce Richardson; dev@dpdk.org
> Subject: Re: [PATCH] librte_lpm: define tbl entry reversely for big
> endian
> 
> 2015-03-09 14:02, Bruce Richardson:
> > On Wed, Mar 04, 2015 at 02:34:12PM +0800, xuelin.shi@freescale.com
> wrote:
> > > From: Xuelin Shi <xuelin.shi@freescale.com>
> > >
> > > This module uses type conversion between struct and int.
> > > Also truncation and comparison is used with this int.
> > > It is not safe for different endian arch.
> > >
> > > Add ifdef for big endian struct to fix this issue.
> > >
> > > Signed-off-by: Xuelin Shi <xuelin.shi@freescale.com>
> >
> > Get an error compiling this up (using clang on FreeBSD).
> >
> >   CC rte_lpm.o
> >   In file included from
> /usr/home/bruce/dpdk.org/lib/librte_lpm/rte_lpm.c:57:
> >   /usr/home/bruce/dpdk.org/lib/librte_lpm/rte_lpm.h:99:5: fatal error:
> > 'RTE_BYTE_ORDER' is not defined, evaluates to 0 [-Wundef] #if
> RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> >       ^
> >       1 error generated.
> >
> > Adding "#include <rte_byteorder.h>" should fix the issue.
> 
> Please Xuelin, could you submit a v2?
> Thanks
> 
> > Existing unit tests on IA (little endian) pass fine there-after, but I
> > think for this patch it would be good to have an ack from someone who
> > can validate on a big endian system, since this is what this patch is
> meant to enable.
> >
> > /Bruce
> >
>
diff mbox

Patch

diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index 1af150c..08a2859 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -96,6 +96,7 @@  extern "C" {
 /** Bitmask used to indicate successful lookup */
 #define RTE_LPM_LOOKUP_SUCCESS          0x0100
 
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
 /** @internal Tbl24 entry structure. */
 struct rte_lpm_tbl24_entry {
 	/* Stores Next hop or group index (i.e. gindex)into tbl8. */
@@ -117,6 +118,24 @@  struct rte_lpm_tbl8_entry {
 	uint8_t valid_group :1; /**< Group validation flag. */
 	uint8_t depth       :6; /**< Rule depth. */
 };
+#else
+struct rte_lpm_tbl24_entry {
+	uint8_t depth 	    :6;
+	uint8_t ext_entry   :1;
+	uint8_t valid	    :1;
+	union {
+		uint8_t tbl8_gindex;
+		uint8_t next_hop;
+	};
+};
+
+struct rte_lpm_tbl8_entry {
+	uint8_t depth 	    :6;
+	uint8_t valid_group :1;
+	uint8_t valid	    :1;
+	uint8_t next_hop;
+};
+#endif
 
 /** @internal Rule structure. */
 struct rte_lpm_rule {