[dpdk-dev] [PATCH] librte_lpm: use field access instead of type conversion.
Hi, Needs more consideration. RTE_LPM_VALID_EXT_ENTRY_BITMASK is defined as 0x0030, also little endian assumed. Seems like the struct bit field also need some position conversion. I would like to send v2 patch to fix that. Thanks, Shi Xuelin > -Original Message- > From: Bruce Richardson [mailto:bruce.richardson at intel.com] > Sent: Thursday, February 12, 2015 19:18 > To: Shi Xuelin-B29237 > Cc: thomas.monjalon at 6wind.com; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH] librte_lpm: use field access instead of > type conversion. > > On Wed, Feb 11, 2015 at 02:12:59PM +0800, xuelin.shi at freescale.com wrote: > > From: Xuelin Shi > > > > struct tbl_entry{ > > uint8_t next_hop; > > uint8_t valid :1; > > uint8_t valid_group :1; > > uint8_t depth :6 > > } > > uint16_t tbl = (uint16_t)tbl_entry; > > next_hop = (uint8_t)tbl; > > > > next_hop cannot get the correct value of the field if the cpu arch is > > BIG_ENDIAN. > > > > change it to field access. > > > > Signed-off-by: Xuelin Shi > > --- > > lib/librte_lpm/rte_lpm.h | 7 +-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h index > > 586300b..1af150c 100644 > > --- a/lib/librte_lpm/rte_lpm.h > > +++ b/lib/librte_lpm/rte_lpm.h > > @@ -273,6 +273,7 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, > > uint8_t *next_hop) { > > unsigned tbl24_index = (ip >> 8); > > uint16_t tbl_entry; > > + struct rte_lpm_tbl8_entry *entry; > > > > /* DEBUG: Check user input arguments. */ > > RTE_LPM_RETURN_IF_TRUE(((lpm == NULL) || (next_hop == NULL)), > > -EINVAL); @@ -290,8 +291,10 @@ rte_lpm_lookup(struct rte_lpm *lpm, > uint32_t ip, uint8_t *next_hop) > > tbl_entry = *(const uint16_t *)>tbl8[tbl8_index]; > > } > > > > - *next_hop = (uint8_t)tbl_entry; > > - return (tbl_entry & RTE_LPM_LOOKUP_SUCCESS) ? 0 : -ENOENT; > > + entry = (struct rte_lpm_tbl8_entry *)_entry; > > + *next_hop = entry->next_hop; > > + > > + return (entry->valid) ? 0 : -ENOENT; > > } > > > > /** > > -- > > 1.9.1 > > > I've run a quick test using "lpm_autotest" inside the test app, and on my > (Intel) platform, this patch has a small (but none-the-less significant) > performance regression. How about the below as an alternative fix? > > /Bruce > > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h index > 586300b..de6f1cb 100644 > --- a/lib/librte_lpm/rte_lpm.h > +++ b/lib/librte_lpm/rte_lpm.h > @@ -44,6 +44,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -287,7 +288,8 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, > uint8_t *next_hop) > unsigned tbl8_index = (uint8_t)ip + > ((uint8_t)tbl_entry * > RTE_LPM_TBL8_GROUP_NUM_ENTRIES); > > - tbl_entry = *(const uint16_t *)>tbl8[tbl8_index]; > + tbl_entry = rte_cpu_to_le_16( > + *(const uint16_t > + *)>tbl8[tbl8_index]); > } > > *next_hop = (uint8_t)tbl_entry;
[dpdk-dev] [PATCH] librte_lpm: use field access instead of type conversion.
On Wed, Feb 11, 2015 at 02:12:59PM +0800, xuelin.shi at freescale.com wrote: > From: Xuelin Shi > > struct tbl_entry{ > uint8_t next_hop; > uint8_t valid :1; > uint8_t valid_group :1; > uint8_t depth :6 > } > uint16_t tbl = (uint16_t)tbl_entry; > next_hop = (uint8_t)tbl; > > next_hop cannot get the correct value of the field > if the cpu arch is BIG_ENDIAN. > > change it to field access. > > Signed-off-by: Xuelin Shi > --- > lib/librte_lpm/rte_lpm.h | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h > index 586300b..1af150c 100644 > --- a/lib/librte_lpm/rte_lpm.h > +++ b/lib/librte_lpm/rte_lpm.h > @@ -273,6 +273,7 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, uint8_t > *next_hop) > { > unsigned tbl24_index = (ip >> 8); > uint16_t tbl_entry; > + struct rte_lpm_tbl8_entry *entry; > > /* DEBUG: Check user input arguments. */ > RTE_LPM_RETURN_IF_TRUE(((lpm == NULL) || (next_hop == NULL)), -EINVAL); > @@ -290,8 +291,10 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, uint8_t > *next_hop) > tbl_entry = *(const uint16_t *)>tbl8[tbl8_index]; > } > > - *next_hop = (uint8_t)tbl_entry; > - return (tbl_entry & RTE_LPM_LOOKUP_SUCCESS) ? 0 : -ENOENT; > + entry = (struct rte_lpm_tbl8_entry *)_entry; > + *next_hop = entry->next_hop; > + > + return (entry->valid) ? 0 : -ENOENT; > } > > /** > -- > 1.9.1 > I've run a quick test using "lpm_autotest" inside the test app, and on my (Intel) platform, this patch has a small (but none-the-less significant) performance regression. How about the below as an alternative fix? /Bruce diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h index 586300b..de6f1cb 100644 --- a/lib/librte_lpm/rte_lpm.h +++ b/lib/librte_lpm/rte_lpm.h @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -287,7 +288,8 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, uint8_t *next_hop) unsigned tbl8_index = (uint8_t)ip + ((uint8_t)tbl_entry * RTE_LPM_TBL8_GROUP_NUM_ENTRIES); - tbl_entry = *(const uint16_t *)>tbl8[tbl8_index]; + tbl_entry = rte_cpu_to_le_16( + *(const uint16_t *)>tbl8[tbl8_index]); } *next_hop = (uint8_t)tbl_entry;
[dpdk-dev] [PATCH] librte_lpm: use field access instead of type conversion.
From: Xuelin Shistruct tbl_entry{ uint8_t next_hop; uint8_t valid :1; uint8_t valid_group :1; uint8_t depth :6 } uint16_t tbl = (uint16_t)tbl_entry; next_hop = (uint8_t)tbl; next_hop cannot get the correct value of the field if the cpu arch is BIG_ENDIAN. change it to field access. Signed-off-by: Xuelin Shi --- lib/librte_lpm/rte_lpm.h | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h index 586300b..1af150c 100644 --- a/lib/librte_lpm/rte_lpm.h +++ b/lib/librte_lpm/rte_lpm.h @@ -273,6 +273,7 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, uint8_t *next_hop) { unsigned tbl24_index = (ip >> 8); uint16_t tbl_entry; + struct rte_lpm_tbl8_entry *entry; /* DEBUG: Check user input arguments. */ RTE_LPM_RETURN_IF_TRUE(((lpm == NULL) || (next_hop == NULL)), -EINVAL); @@ -290,8 +291,10 @@ rte_lpm_lookup(struct rte_lpm *lpm, uint32_t ip, uint8_t *next_hop) tbl_entry = *(const uint16_t *)>tbl8[tbl8_index]; } - *next_hop = (uint8_t)tbl_entry; - return (tbl_entry & RTE_LPM_LOOKUP_SUCCESS) ? 0 : -ENOENT; + entry = (struct rte_lpm_tbl8_entry *)_entry; + *next_hop = entry->next_hop; + + return (entry->valid) ? 0 : -ENOENT; } /** -- 1.9.1