From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>,
"Van Haaren, Harry" <harry.van.haaren@intel.com>,
Stephen Hemminger <stephen@networkplumber.org>,
"Singh, Jasvinder" <jasvinder.singh@intel.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
Date: Mon, 9 Apr 2018 17:05:44 +0000 [thread overview]
Message-ID: <3EB4FA525960D640B5BDFFD6A3D891267BB3D450@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <fb476053-0743-badc-7bcd-4e30e4bd57aa@intel.com>
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, April 9, 2018 5:43 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Singh, Jasvinder
> <jasvinder.singh@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
>
> On 4/9/2018 5:38 PM, Van Haaren, Harry wrote:
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dumitrescu,
> Cristian
> >> Sent: Monday, April 9, 2018 4:59 PM
> >> To: Stephen Hemminger <stephen@networkplumber.org>; Singh,
> Jasvinder
> >> <jasvinder.singh@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> >>> Sent: Monday, April 9, 2018 4:10 PM
> >>> To: Singh, Jasvinder <jasvinder.singh@intel.com>
> >>> Cc: dev@dpdk.org; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> >>> Subject: Re: [dpdk-dev] [PATCH] table: fix build error with gcc 8
> >>>
> >>> On Mon, 9 Apr 2018 13:49:48 +0100
> >>> Jasvinder Singh <jasvinder.singh@intel.com> wrote:
> >>>
> >>>> Fix build error with gcc 8.0 due to cast between function types.
> >>>> Fixes: 5a80bf0ae613 ("table: add cuckoo hash")
> >>>>
> >>>> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> >>>> ---
> >>>> lib/librte_table/rte_table_hash_cuckoo.c | 4 +++-
> >>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/lib/librte_table/rte_table_hash_cuckoo.c
> >>> b/lib/librte_table/rte_table_hash_cuckoo.c
> >>>> index dcb4fe9..f7eae27 100644
> >>>> --- a/lib/librte_table/rte_table_hash_cuckoo.c
> >>>> +++ b/lib/librte_table/rte_table_hash_cuckoo.c
> >>>> @@ -103,11 +103,13 @@ rte_table_hash_cuckoo_create(void
> *params,
> >>>> return NULL;
> >>>> }
> >>>>
> >>>> + void *hash_func = p->f_hash;
> >>>> +
> >>>> /* Create cuckoo hash table */
> >>>> struct rte_hash_parameters hash_cuckoo_params = {
> >>>> .entries = p->n_keys,
> >>>> .key_len = p->key_size,
> >>>> - .hash_func = (rte_hash_function)(p->f_hash),
> >>>> + .hash_func = (rte_hash_function) hash_func,
> >>>> .hash_func_init_val = p->seed,
> >>>> .socket_id = socket_id,
> >>>> .name = p->name
> >>>
> >>> This is just tricking the compiler into not complaining.
> >>> I would really rather see the two hash functions made the same.
> >>
> >> (Adding Bruce as well to consolidate all conversations in a single thread.)
> >>
> >> What we want to do here is be able to use the librte_hash under the
> same API
> >> as the several hash table flavors implemented in librte_table.
> >>
> >> Both of these libraries allow configuring the hash function per each hash
> >> table instance. Problem is: hash function in librte_hash has only 3
> parameters
> >> (no key mask), while hash function in librte_table has 4 parameters
> (includes
> >> key mask). The key mask helps a lot for practical protocol
> implementations by
> >> avoiding key copy & pre-process on lookup.
> >>
> >> So then: how to plug in librte_hash under the same API as the suite of
> hash
> >> tables in librte_table? We don't want to re-implement cuckoo hash from
> >> librte_hash, we simply want to invoke it as a low-level primitive, similarly
> >> to how the LPM and ACL tables are plugged into librte_table.
> >>
> >> Solution is: as an exception, pass a 3-parameter hash function to cuckoo
> hash
> >> flavor under the librte_table. Maybe this should be documented better.
> This
> >> currently triggers a build warning with gcc 8, which is easy to fix, hence
> >> this trivial patch.
> >>
> >> Ideally, for every 3-parameter hash function, I would like to generate the
> >> corresponding 4-parameter hash function on-the-fly, but unfortunately
> this is
> >> not what C language can do.
> >>
> >> Of course, IMO the best solution is to add key mask support to
> librte_hash.
> >
> >
> > Looking at the previous discussion I see the following as a possible
> solution;
> >
> > Given the current code looks broken it should be fixed in this release.
> > Given the actual code fix is an API / ABI break (depending on solution) it
> cannot be merged official in this release.
> > We have a NEXT_ABI macro - it allows us to break API/ABI conditionally at
> compile time.
> >
> > With the above 3 points, I think the best solution is to correctly fix the
> problem that GCC 8 is identifying, and putting that new API inside the NEXT_
> macros.
> >
> > In this case, we can preserve backwards (buggy) behavior if required, and
> provide correct (but API/ABI breaking) code as well. This is a tough decision -
> particularly for distros - what do they package?
>
> +1 to use RTE_NEXT_ABI and deliver fixed code, and agree this is kind of
> pushing
> decision to distros.
>
Again, where is the bug, and where exactly in the code you want to put RTE_NEXT_ABI macro, and what is the problem fixed by using this macro?
As stated in the reply to Harry, this could be reworked in multiple ways if people think the function pointer conversion is misleading to the user.
> >
> > Given the current code, I don't see a better solution - but I hope I'm wrong
> :)
> >
next prev parent reply other threads:[~2018-04-09 17:05 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-09 12:49 Jasvinder Singh
2018-04-09 12:59 ` Dumitrescu, Cristian
2018-04-09 13:09 ` Bruce Richardson
2018-04-09 14:51 ` Singh, Jasvinder
2018-04-09 15:28 ` Bruce Richardson
2018-04-09 16:41 ` Stephen Hemminger
2018-04-09 15:09 ` Stephen Hemminger
2018-04-09 15:58 ` Dumitrescu, Cristian
2018-04-09 16:38 ` Van Haaren, Harry
2018-04-09 16:43 ` Ferruh Yigit
2018-04-09 17:05 ` Dumitrescu, Cristian [this message]
2018-04-09 17:02 ` Dumitrescu, Cristian
2018-04-09 17:09 ` Ananyev, Konstantin
2018-04-09 17:26 ` Dumitrescu, Cristian
2018-04-10 12:32 ` Van Haaren, Harry
2018-04-10 11:43 ` Neil Horman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3EB4FA525960D640B5BDFFD6A3D891267BB3D450@IRSMSX108.ger.corp.intel.com \
--to=cristian.dumitrescu@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=harry.van.haaren@intel.com \
--cc=jasvinder.singh@intel.com \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).