lib: Add hash function for nexthop groups #4066

sworleys · 2019-04-02T18:06:56Z

Add a hash function to turn a nexthop group into a
32 bit unsigned hash key with jhash. We do not care to
hash any recursively resolved nexthops, just the group.

Signed-off-by: Stephen Worley sworley@cumulusnetworks.com

NetDEF-CI · 2019-04-02T19:11:03Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7111/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Ubuntu 16.04 amd64 build: Successful with additional warnings

Debian Package lintian failed for Ubuntu 16.04 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7111/artifact/CI014BUILD/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib: package-has-long-file-name 72 (81) > 80

Ubuntu 18.04 amd64 build: Successful with additional warnings

Debian Package lintian failed for Ubuntu 18.04 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7111/artifact/U1804AMD64/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib: package-has-long-file-name 72 (81) > 80

Ubuntu 16.04 i386 build: Successful with additional warnings

Debian Package lintian failed for Ubuntu 16.04 i386 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7111/artifact/U1604I386/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib: package-has-long-file-name 71 (81) > 80

Debian 9 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 9 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7111/artifact/CI021BUILD/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib-dbgsym: package-has-long-file-name 72 (81) > 80

Ubuntu 14.04 amd64 build: Successful with additional warnings

Debian Package lintian failed for Ubuntu 14.04 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7111/artifact/CI001BUILD/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib: package-has-long-file-name 72 (81) > 80

<TITLE>clang_check</TITLE>

clang_check

LabN-CI · 2019-04-02T19:23:02Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/4066 `9358c78`
Date	04/02/2019
Start	14:59:02
Finish	15:22:50
Run-Time	23:48
Total	1813
Pass	1813
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2019-04-02-14:59:02.txt
Log	autoscript-2019-04-02-14:59:45.log.bz2
Memory	499 491 429

For details, please contact louberger

LabN-CI · 2019-04-02T22:00:08Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/4066 `cf0294a`
Date	04/02/2019
Start	17:35:13
Finish	17:59:43
Run-Time	24:30
Total	1816
Pass	1816
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2019-04-02-17:35:13.txt
Log	autoscript-2019-04-02-17:36:06.log.bz2
Memory	468 491 429

For details, please contact louberger

NetDEF-CI · 2019-04-02T22:39:22Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7112/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Ubuntu 16.04 i386 build: Successful with additional warnings

Debian Package lintian failed for Ubuntu 16.04 i386 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7112/artifact/U1604I386/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib: package-has-long-file-name 71 (81) > 80

Ubuntu 16.04 amd64 build: Successful with additional warnings

Debian Package lintian failed for Ubuntu 16.04 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7112/artifact/CI014BUILD/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib: package-has-long-file-name 72 (81) > 80

Ubuntu 18.04 amd64 build: Successful with additional warnings

Debian Package lintian failed for Ubuntu 18.04 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7112/artifact/U1804AMD64/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib: package-has-long-file-name 72 (81) > 80

Debian 9 amd64 build: Successful with additional warnings

Debian Package lintian failed for Debian 9 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7112/artifact/CI021BUILD/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib-dbgsym: package-has-long-file-name 72 (81) > 80

Ubuntu 14.04 amd64 build: Successful with additional warnings

Debian Package lintian failed for Ubuntu 14.04 amd64 build:
(see full package build log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7112/artifact/CI001BUILD/ErrorLog/log_lintian.txt)

W: frr-rpki-rtrlib: package-has-long-file-name 72 (81) > 80

<TITLE>clang_check</TITLE>

clang_check

louberger

This are suggested changes - if you really don't want to make them, I won't object.

louberger · 2019-04-04T14:24:05Z

lib/nexthop_group.c

+	 */
+	for (nh = nhg->nexthop; nh; nh = nh->next) {
+		key = jhash_1word(nh->type, key);
+		key = jhash_2words(nh->vrf_id, nh->nh_label_type, key);


Why not jhash_3words instead of 1 followed by 2words?

louberger · 2019-04-04T14:30:23Z

lib/nexthop_group.c

+		key = jhash(&nh->rmap_src, sizeof(nh->rmap_src), key);
+		if (nh->nh_label) {
+			for (i = 0; i < nh->nh_label->num_labels; i++)
+				key = jhash_1word(nh->nh_label->label[i], key);


I think it's worth special casing the 2 and 3word case, i.e., something like

i = nh->nh_label->num_labels; while (i >= 3) { key = jhash_3words(nh->nh_label->label[i], nh->nh_label->label[i+1],nh->nh_label->label[i+2],key); i -=3; } if (i >= 2) { key = jhash_2words(nh->nh_label->label[i], nh->nh_label->label[i+1],key); i -=2; } if (i >= 2) { key = jhash_1word(nh->nh_label->label[i], key); i -=2; }

sworleys · 2019-04-05T13:33:11Z

This are suggested changes - if you really don't want to make them, I won't object.

@louberger I implemented your suggested changes and also moved the hash function directly into lib/nexthop.c per @mjstapp 's suggestion offline.

mjstapp

Thanks for moving the logic into the individual nexthop - but I had a concern about the label array indexing.

lib/nexthop.c

NetDEF-CI · 2019-04-05T14:27:33Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7158/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings

Report for nexthop_group.c | 2 issues
===============================================
< WARNING: braces {} are not necessary for single statement blocks
< #160: FILE: /tmp/f1-22250/nexthop_group.c:160:

<TITLE>clang_check</TITLE>

clang_check

LabN-CI · 2019-04-05T14:32:41Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/4066 `955ba95`
Date	04/05/2019
Start	10:08:41
Finish	10:32:28
Run-Time	23:47
Total	1813
Pass	1813
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2019-04-05-10:08:41.txt
Log	autoscript-2019-04-05-10:09:25.log.bz2
Memory	498 497 429

For details, please contact louberger

LabN-CI · 2019-04-05T15:48:03Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/4066 `facf5e9`
Date	04/05/2019
Start	11:24:02
Finish	11:47:50
Run-Time	23:48
Total	1813
Pass	1813
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2019-04-05-11:24:02.txt
Log	autoscript-2019-04-05-11:24:45.log.bz2
Memory	494 501 429

For details, please contact louberger

lib/nexthop.c

lib/nexthop_group.c

louberger · 2019-04-05T16:00:35Z

Yes, sorry bad pseudo code on my part...

…

----------

On April 5, 2019 9:40:02 AM Mark Stapp ***@***.***> wrote: mjstapp requested changes on this pull request. Thanks for moving the logic into the individual nexthop - but I had a concern about the label array indexing. > + if (nexthop->nh_label) { + int i = nexthop->nh_label->num_labels; + + while (i >= 3) { + key = jhash_3words(nexthop->nh_label->label[i], + nexthop->nh_label->label[i + 1], we can't really use `i` this way, can we - aren't we looking beyond the end of the array? I think we need two variables - one counting forward from zero, and a second one counting down the remaining number of labels to process. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #4066 (review)

NetDEF-CI · 2019-04-05T16:17:51Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7162/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings

Report for nexthop_group.c | 2 issues
===============================================
< WARNING: braces {} are not necessary for single statement blocks
< #160: FILE: /tmp/f1-31495/nexthop_group.c:160:

<TITLE>clang_check</TITLE>

clang_check

mjstapp

thanks for the 'const' - looks good

NetDEF-CI · 2019-04-05T17:21:06Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7167/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Warnings Generated during build:

Checkout code: Successful with additional warnings

Report for nexthop_group.c | 2 issues
===============================================
< WARNING: braces {} are not necessary for single statement blocks
< #160: FILE: /tmp/f1-25261/nexthop_group.c:160:

<TITLE>clang_check</TITLE>

clang_check

Add a hash function to turn a nexthop group into a 32 bit unsigned hash key with jhash. We do not care to hash any recursively resolved nexthops, just the group. Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>

LabN-CI · 2019-04-05T18:53:10Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/4066 `1b1fe1c`
Date	04/05/2019
Start	14:29:08
Finish	14:52:57
Run-Time	23:49
Total	1813
Pass	1813
Fail	0
Valgrind-Errors	0
Valgrind-Loss	0
Details	vncregress-2019-04-05-14:29:08.txt
Log	autoscript-2019-04-05-14:29:51.log.bz2
Memory	502 497 429

For details, please contact louberger

NetDEF-CI · 2019-04-05T19:17:36Z

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7168/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

<TITLE>clang_check</TITLE>

clang_check

sworleys force-pushed the Add-NG-Hash-Func branch from 9358c78 to cf0294a Compare April 2, 2019 21:33

louberger requested changes Apr 4, 2019

View reviewed changes

qlyoung added the zebra label Apr 4, 2019

sworleys force-pushed the Add-NG-Hash-Func branch from cf0294a to 955ba95 Compare April 5, 2019 13:22

mjstapp requested changes Apr 5, 2019

View reviewed changes

lib/nexthop.c Show resolved Hide resolved

sworleys force-pushed the Add-NG-Hash-Func branch from 955ba95 to facf5e9 Compare April 5, 2019 15:15

mjstapp reviewed Apr 5, 2019

View reviewed changes

lib/nexthop.c Outdated Show resolved Hide resolved

lib/nexthop_group.c Outdated Show resolved Hide resolved

sworleys force-pushed the Add-NG-Hash-Func branch from facf5e9 to bd495cc Compare April 5, 2019 16:18

mjstapp approved these changes Apr 5, 2019

View reviewed changes

louberger approved these changes Apr 5, 2019

View reviewed changes

lib: Add hash function for nexthop groups

1b1fe1c

Add a hash function to turn a nexthop group into a 32 bit unsigned hash key with jhash. We do not care to hash any recursively resolved nexthops, just the group. Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>

sworleys force-pushed the Add-NG-Hash-Func branch from bd495cc to 1b1fe1c Compare April 5, 2019 18:20

mjstapp merged commit a1a972f into FRRouting:master Apr 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: Add hash function for nexthop groups #4066

lib: Add hash function for nexthop groups #4066

sworleys commented Apr 2, 2019

NetDEF-CI commented Apr 2, 2019

LabN-CI commented Apr 2, 2019

LabN-CI commented Apr 2, 2019

NetDEF-CI commented Apr 2, 2019

louberger left a comment

louberger Apr 4, 2019

louberger Apr 4, 2019 •

edited by qlyoung

Loading

sworleys commented Apr 5, 2019

mjstapp left a comment

NetDEF-CI commented Apr 5, 2019

LabN-CI commented Apr 5, 2019

LabN-CI commented Apr 5, 2019

louberger commented Apr 5, 2019 via email

NetDEF-CI commented Apr 5, 2019

mjstapp left a comment

NetDEF-CI commented Apr 5, 2019

LabN-CI commented Apr 5, 2019

NetDEF-CI commented Apr 5, 2019

lib: Add hash function for nexthop groups #4066

lib: Add hash function for nexthop groups #4066

Conversation

sworleys commented Apr 2, 2019

NetDEF-CI commented Apr 2, 2019

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

clang_check

LabN-CI commented Apr 2, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

LabN-CI commented Apr 2, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Apr 2, 2019

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

clang_check

louberger left a comment

Choose a reason for hiding this comment

louberger Apr 4, 2019

Choose a reason for hiding this comment

louberger Apr 4, 2019 • edited by qlyoung Loading

Choose a reason for hiding this comment

sworleys commented Apr 5, 2019

mjstapp left a comment

Choose a reason for hiding this comment

NetDEF-CI commented Apr 5, 2019

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

clang_check

LabN-CI commented Apr 5, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

LabN-CI commented Apr 5, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

louberger commented Apr 5, 2019 via email

NetDEF-CI commented Apr 5, 2019

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

clang_check

mjstapp left a comment

Choose a reason for hiding this comment

NetDEF-CI commented Apr 5, 2019

Continuous Integration Result: SUCCESSFUL

Warnings Generated during build:

clang_check

LabN-CI commented Apr 5, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

NetDEF-CI commented Apr 5, 2019

Continuous Integration Result: SUCCESSFUL

clang_check

louberger Apr 4, 2019 •

edited by qlyoung

Loading