[dpdk-dev,v5,1/6] librte_table: fix acl entry add and delete functions
Checks
Commit Message
The rte_table_acl_entry_add() function was returning data from
acl_memory instead of acl_rule_memory. It was also returning data
from entry instead of entry_ptr.
The rte_table_acl_entry_delete() function was returning data from
acl_memory instead of acl_rule_memory.
Fixes: 166923eb2f78 ("table: ACL")
Cc: stable@dpdk.org
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
lib/librte_table/rte_table_acl.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Comments
Hi Bernard,
<snip>
> --- a/lib/librte_table/rte_table_acl.c
> +++ b/lib/librte_table/rte_table_acl.c
> @@ -316,8 +316,7 @@ struct rte_table_acl {
> if (status == 0) {
> *key_found = 1;
> *entry_ptr = &acl->memory[i * acl->entry_size];
> - memcpy(*entry_ptr, entry, acl->entry_size);
> -
> + memcpy(entry, *entry_ptr, acl->entry_size);
> return 0;
> }
> }
In this case, table entry which is being added already presents in the table. So, first the pointer to that entry from the memory[] that stores the pipeline table data(struct rte_pipeline_table_entry) associated with key is retrieved and the changes (action and metadara) are stored in the internal table pointed by action_table. So, above fix doesn't seem correct.
> @@ -353,8 +352,8 @@ struct rte_table_acl {
> rte_acl_free(acl->ctx);
> acl->ctx = ctx;
> *key_found = 0;
> - *entry_ptr = &acl->memory[free_pos * acl->entry_size];
> - memcpy(*entry_ptr, entry, acl->entry_size);
> + *entry_ptr = &acl->acl_rule_memory[free_pos * acl->entry_size];
> + memcpy(entry, *entry_ptr, acl->entry_size);
>
> return 0;
> }
> @@ -435,7 +434,7 @@ struct rte_table_acl {
> acl->ctx = ctx;
> *key_found = 1;
> if (entry != NULL)
> - memcpy(entry, &acl->memory[pos * acl->entry_size],
> + memcpy(entry, &acl->acl_rule_memory[pos * acl-
> >entry_size],
> acl->entry_size);
Above fixes also seems not correct. As per documentation, *entry_ptr is intended to store the handle to the pipeline table entry containing the data associated with the current key instead of pointing to memory used to store the acl rules, etc. Please refer rte_table_acl_create() where memory is initialized and organized to stores different types of internal tables (pointed by action_table, acl_rule_list and acl_rule_memory).
> -----Original Message-----
> From: Singh, Jasvinder
> Sent: Monday, September 18, 2017 4:30 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
> Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; adrien.mazarguil@6wind.com
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add and
> delete functions
>
> Hi Bernard,
>
> <snip>
>
> > --- a/lib/librte_table/rte_table_acl.c
> > +++ b/lib/librte_table/rte_table_acl.c
> > @@ -316,8 +316,7 @@ struct rte_table_acl {
> > if (status == 0) {
> > *key_found = 1;
> > *entry_ptr = &acl->memory[i * acl->entry_size];
> > - memcpy(*entry_ptr, entry, acl->entry_size);
> > -
> > + memcpy(entry, *entry_ptr, acl->entry_size);
> > return 0;
> > }
> > }
>
> In this case, table entry which is being added already presents in the table.
> So, first the pointer to that entry from the memory[] that stores the pipeline
> table data(struct rte_pipeline_table_entry) associated with key is retrieved
> and the changes (action and metadara) are stored in the internal table
> pointed by action_table. So, above fix doesn't seem correct.
>
> > @@ -353,8 +352,8 @@ struct rte_table_acl {
> > rte_acl_free(acl->ctx);
> > acl->ctx = ctx;
> > *key_found = 0;
> > - *entry_ptr = &acl->memory[free_pos * acl->entry_size];
> > - memcpy(*entry_ptr, entry, acl->entry_size);
> > + *entry_ptr = &acl->acl_rule_memory[free_pos * acl->entry_size];
> > + memcpy(entry, *entry_ptr, acl->entry_size);
> >
> > return 0;
> > }
> > @@ -435,7 +434,7 @@ struct rte_table_acl {
> > acl->ctx = ctx;
> > *key_found = 1;
> > if (entry != NULL)
> > - memcpy(entry, &acl->memory[pos * acl->entry_size],
> > + memcpy(entry, &acl->acl_rule_memory[pos * acl-
> > >entry_size],
> > acl->entry_size);
>
>
> Above fixes also seems not correct. As per documentation, *entry_ptr is
> intended to store the handle to the pipeline table entry containing the data
> associated with the current key instead of pointing to memory used to store
> the acl rules, etc. Please refer rte_table_acl_create() where memory is
> initialized and organized to stores different types of internal tables (pointed
> by action_table, acl_rule_list and acl_rule_memory).
NACK
Fully agree with Jasvinder.
Existing code is correct, proposed code changes are wrong.
Hi Cristian,
> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Wednesday, September 20, 2017 1:21 PM
> To: Singh, Jasvinder <jasvinder.singh@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; dev@dpdk.org; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; adrien.mazarguil@6wind.com
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add and
> delete functions
>
>
>
> > -----Original Message-----
> > From: Singh, Jasvinder
> > Sent: Monday, September 18, 2017 4:30 PM
> > To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
> > Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; adrien.mazarguil@6wind.com
> > Cc: Iremonger, Bernard <bernard.iremonger@intel.com>;
> stable@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add
> > and delete functions
> >
> > Hi Bernard,
> >
> > <snip>
> >
> > > --- a/lib/librte_table/rte_table_acl.c
> > > +++ b/lib/librte_table/rte_table_acl.c
> > > @@ -316,8 +316,7 @@ struct rte_table_acl {
> > > if (status == 0) {
> > > *key_found = 1;
> > > *entry_ptr = &acl->memory[i * acl->entry_size];
> > > - memcpy(*entry_ptr, entry, acl->entry_size);
> > > -
> > > + memcpy(entry, *entry_ptr, acl->entry_size);
> > > return 0;
> > > }
> > > }
> >
> > In this case, table entry which is being added already presents in the table.
> > So, first the pointer to that entry from the memory[] that stores the
> > pipeline table data(struct rte_pipeline_table_entry) associated with
> > key is retrieved and the changes (action and metadara) are stored in
> > the internal table pointed by action_table. So, above fix doesn't seem
> correct.
> >
> > > @@ -353,8 +352,8 @@ struct rte_table_acl {
> > > rte_acl_free(acl->ctx);
> > > acl->ctx = ctx;
> > > *key_found = 0;
> > > - *entry_ptr = &acl->memory[free_pos * acl->entry_size];
> > > - memcpy(*entry_ptr, entry, acl->entry_size);
> > > + *entry_ptr = &acl->acl_rule_memory[free_pos * acl->entry_size];
> > > + memcpy(entry, *entry_ptr, acl->entry_size);
> > >
> > > return 0;
> > > }
> > > @@ -435,7 +434,7 @@ struct rte_table_acl {
> > > acl->ctx = ctx;
> > > *key_found = 1;
> > > if (entry != NULL)
> > > - memcpy(entry, &acl->memory[pos * acl->entry_size],
> > > + memcpy(entry, &acl->acl_rule_memory[pos * acl-
> > > >entry_size],
> > > acl->entry_size);
> >
> >
> > Above fixes also seems not correct. As per documentation, *entry_ptr
> > is intended to store the handle to the pipeline table entry containing
> > the data associated with the current key instead of pointing to memory
> > used to store the acl rules, etc. Please refer rte_table_acl_create()
> > where memory is initialized and organized to stores different types of
> > internal tables (pointed by action_table, acl_rule_list and
> acl_rule_memory).
>
> NACK
>
> Fully agree with Jasvinder.
>
> Existing code is correct, proposed code changes are wrong.
I will drop this patch and send a v6 patchset.
Regards,
Bernard.
@@ -316,8 +316,7 @@ struct rte_table_acl {
if (status == 0) {
*key_found = 1;
*entry_ptr = &acl->memory[i * acl->entry_size];
- memcpy(*entry_ptr, entry, acl->entry_size);
-
+ memcpy(entry, *entry_ptr, acl->entry_size);
return 0;
}
}
@@ -353,8 +352,8 @@ struct rte_table_acl {
rte_acl_free(acl->ctx);
acl->ctx = ctx;
*key_found = 0;
- *entry_ptr = &acl->memory[free_pos * acl->entry_size];
- memcpy(*entry_ptr, entry, acl->entry_size);
+ *entry_ptr = &acl->acl_rule_memory[free_pos * acl->entry_size];
+ memcpy(entry, *entry_ptr, acl->entry_size);
return 0;
}
@@ -435,7 +434,7 @@ struct rte_table_acl {
acl->ctx = ctx;
*key_found = 1;
if (entry != NULL)
- memcpy(entry, &acl->memory[pos * acl->entry_size],
+ memcpy(entry, &acl->acl_rule_memory[pos * acl->entry_size],
acl->entry_size);
return 0;