Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1450 closed enhancement (complete)

DHCP benchmarking utility

Reported by: fdupont Owned by: UnAssigned
Priority: medium Milestone: Sprint-DHCP-20111230
Component: perfdhcp Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 20 Add Hours to Ticket: 0
Total Hours: 20 Internal?: no

Description (last modified by stephen)

Update the design document about the DHCP benchmarking utility, cf DhcpBenchmarking. I suggest everything about the utility itself to be discussed here.

Also covers the implementation of the utility.

Subtickets

Change History (13)

comment:1 Changed 8 years ago by stephen

  • Milestone changed from DHCP 2011 to Sprint-DHCP-20111207

comment:2 Changed 8 years ago by stephen

  • Owner set to fdupont
  • Status changed from new to assigned

comment:3 Changed 8 years ago by fdupont

I created a branch (trac1450) and committed the initial code for the phase 1 (aka 2 packet exchanges, vs phase 2 aka 4 packet exchanges).
It compiles on FreeBSD but was tested on Ubuntu only.
BTW on Linux it needs -lrt -lm

comment:4 follow-up: Changed 8 years ago by stephen

Reviewed commit 6f615f4d947042151a35f8e8184564a0cd08052c

Specific points:

I suggest that the #define DHCPxx are placed into a separate header file. Ultimately the code will use the same values but from a header files used in the other DHCP code (thus having all the definitions in one place).

I suggest that the ISC_TAILQ macros are also placed into a separate header file, as they tend to detract from the content of the file.

Could we have a brief description of the data structures used? I can see that there is a global linked list and a linked list used as a hash table but why there are there is not clear.

Do we have a reference as to how the randomisation performed - it's beyond a simple call to random().

Can we have a brief description on how to use template files?

For safety, the automatic variables should be initialized when declared. I have tried to use perfdhcp between two of my (Ubuntu) virtual machines with the command:

./perfdhcp -4 -i 192.168.7.100

... and it failed with

bad server=192.168.7.100: Bad value for ai_flags

I think this is due to the fact that "flags" declared in main() has not been initialised. It does not appear to be set in all paths through the code, yet is passed to getserveraddr() where it is ORed with what appear to be valid values and passed to getaddrinfo().

The code does not conform to the BIND 10 coding or commenting standards. However, don't worry about this - I'll take out a ticket and add the formatting and comments after you finish phase 2. However, it would be useful if you could add a comment that gives a brief description of each function.

comment:5 in reply to: ↑ 4 Changed 8 years ago by fdupont

Replying to stephen:

Reviewed commit 6f615f4d947042151a35f8e8184564a0cd08052c

Specific points:

I suggest that the #define DHCPxx are placed into a separate header file. Ultimately the code will use the same values but from a header files used in the other DHCP code (thus having all the definitions in one place).

=> there is no such file in KEA (only a copy of ISC DHCPv4 files, including some typos),
so I selected some defines and copied them. Of course, it should be updated when such
header files will be available.

I suggest that the ISC_TAILQ macros are also placed into a separate header file, as they tend to detract from the content of the file.

=> usually it is enough to include a generic ISC file but as BIND10 is written in C++
there is no such file. BTW I copied the FreeBSD 8.2 /sys/sys/queue.h so the defines
have nothing special at the exception of the ISC_ prefix.

Could we have a brief description of the data structures used? I can see that there is a global linked list and a linked list used as a hash table but why there are there is not clear.

=> here or in the code. To summary there are 2 (4 in phase 2) queues and one (2) hash table:

  • one queue for sent messages, and one for received messages. The exchange structure is moved from the sent queue to the received queue when a received message matches, so at the end the sent queue holds the initiated but not answered exchanges, and the received queue the completed exchanges
  • the hash table is indexed by the xid (aka. transaction ID, odd/even in phase 2), and is mainly used to find quickly the matching exchange on reception
  • there is also a pointer to the expected next exchange, of course it works well only with a low loss rate

Do we have a reference as to how the randomisation performed - it's beyond a simple call to random().

=> I don't know if the question is about randomization itself or how it is used,
so I answer to both:

  • the random() function is a Monte Carlo function, i.e., it has good statistical properties but is poor for crypto. IMHO it is exactly what we need. There is a way to get the seed so a run can be reproduced (with a given DUID in DHCPv6) with exactly the same random drawing
  • randomization takes a base (i.e., what to randomize) and a range (i.e., how to randomize). Currently it is MAC address or DUID, an uniform (i.e., with the same probability for each value) random value is added at the end of the base, octet by octet with a carry in the last 4 octets. For instance if the last octet of the base is 1 and range is 99 (i.e., 0..99) then the last octet will get a value from 1 to 100 with a .01 probability.

Can we have a brief description on how to use template files?

=> use "-x T", put the content in a file and convert the parameters into command file
arguments (this is how I tested it).

For safety, the automatic variables should be initialized when declared.

=> the (ai_)flags bug is fixed

The code does not conform to the BIND 10 coding or commenting standards.

=> yes, it is C, not C++, and without doxygen...

However, don't worry about this - I'll take out a ticket and add the formatting and
comments after you finish phase 2. However, it would be useful if you could add
a comment that gives a brief description of each function.

=> I prefer to finish the code (and the wiki/usage) first.

PS: I don't know if the wiki is correctly protected against concurrent changes?

comment:6 Changed 8 years ago by tomek

  • Component changed from dhcp to perfdhcp

comment:7 Changed 8 years ago by fdupont

Phase 2 (on 2 :-) initial code ready (i.e., it was committed and is available for review).
BTW I am looking for ideas to improve it (other than to add comments), in particular
for the sending timings (and note I tried it only on virtual machines so perhaps I got
a biased view on how it (doesn't) work well).

comment:8 Changed 8 years ago by fdupont

Beta ready, hash 2611ea7..fc05832

comment:9 Changed 8 years ago by stephen

  • Description modified (diff)
  • Summary changed from DHCP benchmarking utility: revamping design to DHCP benchmarking utility

comment:10 Changed 8 years ago by stephen

  • Owner changed from fdupont to UnAssigned
  • Status changed from assigned to reviewing

Code OK for now, although will need refactoring and unit tests to fit in with BIND 10 standards. (For that reason, although the functionality is there, I propose to call it an alpha release.) In the absence of Francis, I propose to merge it into master with the ChangeLog message:

xxx.	[func]		fdupont
	Alpha version of DHCP benchmarking added.  "perfdhcp" is able to test both IPv4
        and IPv6 servers: it can time the four-way packet exchange (DORA and SARR) as
        well as time the initial two-packet exchange (DO and SA).  More information can
        be obtained by invoking the utility (in tests/tools/perfdhcp) with the -h flag.
	(Trac #1450, git xxx)

comment:11 Changed 8 years ago by shane

This ChangeLog entry seems fine to me.

Last edited 8 years ago by shane (previous) (diff)

comment:12 Changed 8 years ago by stephen

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged into master in commit 85083a76107ba2236732b45524ce7018eefbaf90

comment:13 Changed 8 years ago by stephen

Note: refactoring ticket is #1268.

Note: See TracTickets for help on using tickets.