[dpdk-dev] examples/ip_pipeline: avoid the failure of creating hash table
Checks
Commit Message
Hash table function will check if the input bucket size is power of 2,
so the parameter should be rounded up before sending to the creating function.
Signed-off-by: Jianbo Liu <jianbo.liu@arm.com>
---
examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c | 2 +-
examples/ip_pipeline/pipeline/pipeline_routing_be.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
Comments
> -----Original Message-----
> From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> Sent: Friday, October 27, 2017 3:55 AM
> To: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Jianbo Liu <jianbo.liu@arm.com>
> Subject: [PATCH] examples/ip_pipeline: avoid the failure of creating hash
> table
>
> Hash table function will check if the input bucket size is power of 2,
> so the parameter should be rounded up before sending to the creating
> function.
>
> Signed-off-by: Jianbo Liu <jianbo.liu@arm.com>
> ---
> examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c | 2 +-
> examples/ip_pipeline/pipeline/pipeline_routing_be.c | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
Existing code is simply letting the library detect the misconfiguration and gracefully fail. It avoids duplicating library checks in the app.
Your proposal tries to prevent library from failing by silently tweaking some user configuration params. Easier to debug in some cases.
For this case, I am OK with your proposal, although not really required, so:
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
The 10/27/2017 10:01, Dumitrescu, Cristian wrote:
>
>
> > -----Original Message-----
> > From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> > Sent: Friday, October 27, 2017 3:55 AM
> > To: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > Cc: Jianbo Liu <jianbo.liu@arm.com>
> > Subject: [PATCH] examples/ip_pipeline: avoid the failure of creating hash
> > table
> >
> > Hash table function will check if the input bucket size is power of 2,
> > so the parameter should be rounded up before sending to the creating
> > function.
> >
> > Signed-off-by: Jianbo Liu <jianbo.liu@arm.com>
> > ---
> > examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c | 2 +-
> > examples/ip_pipeline/pipeline/pipeline_routing_be.c | 3 ++-
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
>
> Existing code is simply letting the library detect the misconfiguration and gracefully fail. It avoids duplicating library checks in the app.
>
> Your proposal tries to prevent library from failing by silently tweaking some user configuration params. Easier to debug in some cases.
Yes. but is it must for the parameters to be power of 2? I saw the
config exmple in examples/ip_pipeline/config/network_layers.cfg:
....
178 port_local_dest = 4 ; SINK2 (Drop)
179 n_arp_entries = 1000
180 ip_hdr_offset = 270
If not, it's the programmer to correct it before sending to the library.
Thanks!
Jianbo
>
> For this case, I am OK with your proposal, although not really required, so:
>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> -----Original Message-----
> From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> Sent: Monday, October 30, 2017 3:33 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] examples/ip_pipeline: avoid the failure of creating hash
> table
>
> The 10/27/2017 10:01, Dumitrescu, Cristian wrote:
> >
> >
> > > -----Original Message-----
> > > From: Jianbo Liu [mailto:jianbo.liu@arm.com]
> > > Sent: Friday, October 27, 2017 3:55 AM
> > > To: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> > > Cc: Jianbo Liu <jianbo.liu@arm.com>
> > > Subject: [PATCH] examples/ip_pipeline: avoid the failure of creating hash
> > > table
> > >
> > > Hash table function will check if the input bucket size is power of 2,
> > > so the parameter should be rounded up before sending to the creating
> > > function.
> > >
> > > Signed-off-by: Jianbo Liu <jianbo.liu@arm.com>
> > > ---
> > > examples/ip_pipeline/pipeline/pipeline_flow_classification_be.c | 2 +-
> > > examples/ip_pipeline/pipeline/pipeline_routing_be.c | 3 ++-
> > > 2 files changed, 3 insertions(+), 2 deletions(-)
> > >
> >
> > Existing code is simply letting the library detect the misconfiguration and
> gracefully fail. It avoids duplicating library checks in the app.
> >
> > Your proposal tries to prevent library from failing by silently tweaking some
> user configuration params. Easier to debug in some cases.
>
> Yes. but is it must for the parameters to be power of 2? I saw the
> config exmple in examples/ip_pipeline/config/network_layers.cfg:
>
> ....
> 178 port_local_dest = 4 ; SINK2 (Drop)
> 179 n_arp_entries = 1000
> 180 ip_hdr_offset = 270
>
> If not, it's the programmer to correct it before sending to the library.
>
> Thanks!
> Jianbo
>
In the latest code, the number of keys does not have to be a power of 2, while the number of buckets does.
I am considering changing the implementation to quietly upgrade number of buckets to the next power of 2 to eliminate some of these error cases.
+: less things for the (unengaged) user to worry about
-: number of buckets needs to be a power of two internally, so a non-power of 2 is likely a misconfiguration; the memory footprint can get much higher when quiet upgrade takes place
> >
> > For this case, I am OK with your proposal, although not really required, so:
> >
> > Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> >
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
On 10/27/2017 3:01 AM, Dumitrescu, Cristian wrote:
>
>
>> -----Original Message-----
>> From: Jianbo Liu [mailto:jianbo.liu@arm.com]
>> Sent: Friday, October 27, 2017 3:55 AM
>> To: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
>> Cc: Jianbo Liu <jianbo.liu@arm.com>
>> Subject: [PATCH] examples/ip_pipeline: avoid the failure of creating hash
>> table
>>
>> Hash table function will check if the input bucket size is power of 2,
>> so the parameter should be rounded up before sending to the creating
>> function.
>>
>> Signed-off-by: Jianbo Liu <jianbo.liu@arm.com>
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Applied to dpdk/master, thanks.
@@ -499,7 +499,7 @@ static void *pipeline_fc_init(struct pipeline_params *params,
.key_mask = (p_fc->key_mask_present) ?
p_fc->key_mask : NULL,
.n_keys = p_fc->n_flows,
- .n_buckets = p_fc->n_flows / 4,
+ .n_buckets = rte_align32pow2(p_fc->n_flows / 4),
.f_hash = hash_func[(p_fc->key_size / 8) - 1],
.seed = 0,
};
@@ -1355,7 +1355,8 @@ struct arp_table_entry {
.key_offset = p_rt->params.arp_key_offset,
.key_mask = NULL,
.n_keys = p_rt->params.n_arp_entries,
- .n_buckets = p_rt->params.n_arp_entries / 4,
+ .n_buckets =
+ rte_align32pow2(p_rt->params.n_arp_entries / 4),
.f_hash = hash_default_key8,
.seed = 0,
};