* [dpdk-stable] [PATCH v5 1/6] librte_table: fix acl entry add and delete functions
[not found] <1504693668-2062-1-git-send-email-bernard.iremonger@intel.com>
@ 2017-09-07 16:43 ` Bernard Iremonger
2017-09-18 15:29 ` [dpdk-stable] [dpdk-dev] " Singh, Jasvinder
2017-09-07 16:43 ` [dpdk-stable] [PATCH v5 2/6] librte_table: fix acl lookup function Bernard Iremonger
1 sibling, 1 reply; 7+ messages in thread
From: Bernard Iremonger @ 2017-09-07 16:43 UTC (permalink / raw)
To: dev, ferruh.yigit, konstantin.ananyev, cristian.dumitrescu,
adrien.mazarguil
Cc: Bernard Iremonger, stable
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(-)
diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
index 3c05e4a..e84b437 100644
--- 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;
}
}
@@ -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;
--
1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [dpdk-stable] [PATCH v5 2/6] librte_table: fix acl lookup function
[not found] <1504693668-2062-1-git-send-email-bernard.iremonger@intel.com>
2017-09-07 16:43 ` [dpdk-stable] [PATCH v5 1/6] librte_table: fix acl entry add and delete functions Bernard Iremonger
@ 2017-09-07 16:43 ` Bernard Iremonger
2017-09-20 12:24 ` Dumitrescu, Cristian
1 sibling, 1 reply; 7+ messages in thread
From: Bernard Iremonger @ 2017-09-07 16:43 UTC (permalink / raw)
To: dev, ferruh.yigit, konstantin.ananyev, cristian.dumitrescu,
adrien.mazarguil
Cc: Bernard Iremonger, stable
The rte_table_acl_lookup() 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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
index e84b437..258916d 100644
--- a/lib/librte_table/rte_table_acl.c
+++ b/lib/librte_table/rte_table_acl.c
@@ -794,7 +794,7 @@ struct rte_table_acl {
if (action_table_pos != 0) {
pkts_out_mask |= pkt_mask;
entries[pkt_pos] = (void *)
- &acl->memory[action_table_pos *
+ &acl->acl_rule_memory[action_table_pos *
acl->entry_size];
rte_prefetch0(entries[pkt_pos]);
}
--
1.9.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add and delete functions
2017-09-07 16:43 ` [dpdk-stable] [PATCH v5 1/6] librte_table: fix acl entry add and delete functions Bernard Iremonger
@ 2017-09-18 15:29 ` Singh, Jasvinder
2017-09-20 12:21 ` Dumitrescu, Cristian
0 siblings, 1 reply; 7+ messages in thread
From: Singh, Jasvinder @ 2017-09-18 15:29 UTC (permalink / raw)
To: Iremonger, Bernard, dev, Yigit, Ferruh, Ananyev, Konstantin,
Dumitrescu, Cristian, adrien.mazarguil
Cc: Iremonger, Bernard, stable
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).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add and delete functions
2017-09-18 15:29 ` [dpdk-stable] [dpdk-dev] " Singh, Jasvinder
@ 2017-09-20 12:21 ` Dumitrescu, Cristian
2017-09-29 8:25 ` Iremonger, Bernard
0 siblings, 1 reply; 7+ messages in thread
From: Dumitrescu, Cristian @ 2017-09-20 12:21 UTC (permalink / raw)
To: Singh, Jasvinder, Iremonger, Bernard, dev, Yigit, Ferruh,
Ananyev, Konstantin, adrien.mazarguil
Cc: Iremonger, Bernard, stable
> -----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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-stable] [PATCH v5 2/6] librte_table: fix acl lookup function
2017-09-07 16:43 ` [dpdk-stable] [PATCH v5 2/6] librte_table: fix acl lookup function Bernard Iremonger
@ 2017-09-20 12:24 ` Dumitrescu, Cristian
2017-09-29 8:27 ` Iremonger, Bernard
0 siblings, 1 reply; 7+ messages in thread
From: Dumitrescu, Cristian @ 2017-09-20 12:24 UTC (permalink / raw)
To: Iremonger, Bernard, dev, Yigit, Ferruh, Ananyev, Konstantin,
adrien.mazarguil
Cc: stable
> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Thursday, September 7, 2017 5:43 PM
> To: 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: [PATCH v5 2/6] librte_table: fix acl lookup function
>
> The rte_table_acl_lookup() 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/librte_table/rte_table_acl.c b/lib/librte_table/rte_table_acl.c
> index e84b437..258916d 100644
> --- a/lib/librte_table/rte_table_acl.c
> +++ b/lib/librte_table/rte_table_acl.c
> @@ -794,7 +794,7 @@ struct rte_table_acl {
> if (action_table_pos != 0) {
> pkts_out_mask |= pkt_mask;
> entries[pkt_pos] = (void *)
> - &acl->memory[action_table_pos *
> + &acl->acl_rule_memory[action_table_pos *
> acl->entry_size];
> rte_prefetch0(entries[pkt_pos]);
> }
> --
> 1.9.1
NACK
Existing code is correct, proposed code changes are wrong (for same reasons described in patch 1 of this patch set).
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH v5 1/6] librte_table: fix acl entry add and delete functions
2017-09-20 12:21 ` Dumitrescu, Cristian
@ 2017-09-29 8:25 ` Iremonger, Bernard
0 siblings, 0 replies; 7+ messages in thread
From: Iremonger, Bernard @ 2017-09-29 8:25 UTC (permalink / raw)
To: Dumitrescu, Cristian, Singh, Jasvinder, dev, Yigit, Ferruh,
Ananyev, Konstantin, adrien.mazarguil
Cc: stable
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-stable] [PATCH v5 2/6] librte_table: fix acl lookup function
2017-09-20 12:24 ` Dumitrescu, Cristian
@ 2017-09-29 8:27 ` Iremonger, Bernard
0 siblings, 0 replies; 7+ messages in thread
From: Iremonger, Bernard @ 2017-09-29 8:27 UTC (permalink / raw)
To: Dumitrescu, Cristian, dev, Yigit, Ferruh, Ananyev, Konstantin,
adrien.mazarguil
Cc: stable
Hi Cristian,
> -----Original Message-----
> From: Dumitrescu, Cristian
> Sent: Wednesday, September 20, 2017 1:24 PM
> To: 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: stable@dpdk.org
> Subject: RE: [PATCH v5 2/6] librte_table: fix acl lookup function
>
>
>
> > -----Original Message-----
> > From: Iremonger, Bernard
> > Sent: Thursday, September 7, 2017 5:43 PM
> > To: 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: [PATCH v5 2/6] librte_table: fix acl lookup function
> >
> > The rte_table_acl_lookup() 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 | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_table/rte_table_acl.c
> > b/lib/librte_table/rte_table_acl.c
> > index e84b437..258916d 100644
> > --- a/lib/librte_table/rte_table_acl.c
> > +++ b/lib/librte_table/rte_table_acl.c
> > @@ -794,7 +794,7 @@ struct rte_table_acl {
> > if (action_table_pos != 0) {
> > pkts_out_mask |= pkt_mask;
> > entries[pkt_pos] = (void *)
> > - &acl->memory[action_table_pos *
> > + &acl->acl_rule_memory[action_table_pos *
> > acl->entry_size];
> > rte_prefetch0(entries[pkt_pos]);
> > }
> > --
> > 1.9.1
>
> NACK
>
> Existing code is correct, proposed code changes are wrong (for same reasons
> described in patch 1 of this patch set).
I will drop this patch and send a v6 patch set.
Regards,
Bernard.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-09-29 8:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1504693668-2062-1-git-send-email-bernard.iremonger@intel.com>
2017-09-07 16:43 ` [dpdk-stable] [PATCH v5 1/6] librte_table: fix acl entry add and delete functions Bernard Iremonger
2017-09-18 15:29 ` [dpdk-stable] [dpdk-dev] " Singh, Jasvinder
2017-09-20 12:21 ` Dumitrescu, Cristian
2017-09-29 8:25 ` Iremonger, Bernard
2017-09-07 16:43 ` [dpdk-stable] [PATCH v5 2/6] librte_table: fix acl lookup function Bernard Iremonger
2017-09-20 12:24 ` Dumitrescu, Cristian
2017-09-29 8:27 ` Iremonger, Bernard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).