Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2)

Issue 6741002: Driver changes

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by yilongl
Modified:
4 years, 11 months ago
Reviewers:
ouster
CC:
behnam.mn
Visibility:
Public.

Description

Driver changes

Patch Set 1 #

Patch Set 2 : Driver changes #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+392 lines, -183 lines) Patch
M GNUmakefile View 1 chunk +1 line, -1 line 0 comments Download
M src/BasicTransport.h View 2 chunks +6 lines, -2 lines 0 comments Download
M src/BasicTransport.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M src/BasicTransportTest.cc View 3 chunks +7 lines, -7 lines 0 comments Download
M src/BoostIntrusive.h View 2 chunks +4 lines, -4 lines 0 comments Download
M src/DpdkDriver.h View 1 5 chunks +41 lines, -24 lines 3 comments Download
M src/DpdkDriver.cc View 1 17 chunks +170 lines, -84 lines 6 comments Download
M src/DpdkDriverTest.cc View 1 3 chunks +65 lines, -3 lines 1 comment Download
M src/Driver.h View 6 chunks +27 lines, -8 lines 2 comments Download
M src/InfUdDriver.h View 2 chunks +6 lines, -5 lines 0 comments Download
M src/InfUdDriver.cc View 6 chunks +15 lines, -8 lines 0 comments Download
M src/MockDriver.h View 2 chunks +5 lines, -4 lines 0 comments Download
M src/MockDriver.cc View 3 chunks +7 lines, -6 lines 0 comments Download
M src/NetUtil.h View 2 chunks +4 lines, -5 lines 0 comments Download
M src/SolarFlareDriver.h View 2 chunks +5 lines, -4 lines 0 comments Download
M src/SolarFlareDriver.cc View 3 chunks +5 lines, -4 lines 0 comments Download
M src/UdpDriver.h View 2 chunks +6 lines, -5 lines 0 comments Download
M src/UdpDriver.cc View 5 chunks +15 lines, -6 lines 1 comment Download

Messages

Total messages: 5
yilongl
This patch does two major things: * Add priority to the Driver interface * Actually ...
4 years, 11 months ago (2017-02-06 23:23:27 UTC) #1
yilongl
On 2017/02/06 23:23:27, yilongl wrote: > This patch does two major things: > > * ...
4 years, 11 months ago (2017-02-07 05:15:58 UTC) #2
ouster
OK; I'll wait. -John- On Mon, Feb 6, 2017 at 9:15 PM, <yilongl@stanford.edu> wrote: > ...
4 years, 11 months ago (2017-02-07 16:52:52 UTC) #3
yilongl
I have uploaded a second patch set. This is now ready for review. Thanks, Yilong ...
4 years, 11 months ago (2017-02-08 22:00:38 UTC) #4
ouster
4 years, 11 months ago (2017-02-14 22:58:47 UTC) #5
Looks good overall; let's get together to discuss (briefly) the comments marked
"let's discuss".

-John-

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.cc
File src/DpdkDriver.cc (right):

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.cc#newcode186
src/DpdkDriver.cc:186: // Reading the MAC address from the NIC via DPDK.
Reading => Read

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.cc#newcode189
src/DpdkDriver.cc:189: locatorString = format("dpdk:mac=%s",
localMac->toString().c_str());
Let's discuss how this locator string is used. I think this change might be a
good idea, but it needs to be documented more thoroughly (e.g. you've changed
the definition of Driver::getServiceLocator, no?).

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.cc#newcode193
src/DpdkDriver.cc:193: portConf.rxmode.max_rx_pkt_len =
ETHER_MAX_VLAN_FRAME_LEN;
Where is this defined, and why not use MAX_PAYLOAD_size instead?  Let's discuss.

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.cc#newcode248
src/DpdkDriver.cc:248: ret = rte_eth_dev_set_mtu(portId, ETHER_MTU);
Same question as for line 193.

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.cc#newcode462
src/DpdkDriver.cc:462: uint16_t vlan_tci =
downCast<uint16_t>(PRIORITY_TO_PCP[priority] << 13);
Why not make PRIORITY_TO_PCP map directly to the shifted values, so you don't
need "<< 13" here?

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.cc#newcode486
src/DpdkDriver.cc:486: &mockEtherFrame[ETHER_VLAN_HDR_LEN]);
Would it be easier to write tests if the body of the packet was a string, rather
than hexadecimal bytes?

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.h
File src/DpdkDriver.h (right):

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.h#newcode84
src/DpdkDriver.h:84: /// Size of VLAN tag, in bytes.
Need a note in this comment to remind readers that we use the VLAN tag to
specify the packet priority. Otherwise people won't understand why this code is
here.

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.h#newcode92
src/DpdkDriver.h:92: /// actually the lowest priority, while PCP = 0 is the
second lowest.
This is a very strange allocation; let's discuss.

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriver.h#newcode136
src/DpdkDriver.h:136: int lowestPriorityAvail;
This value isn't exposed to DpdkDriver clients; what if a client specifies a
value lower than this when sending a packet? Let's discuss.

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriverTest.cc
File src/DpdkDriverTest.cc (right):

https://rccodereview.appspot.com/6741002/diff/20001/src/DpdkDriverTest.cc#new...
src/DpdkDriverTest.cc:98: }
Thanks for adding more tests.

https://rccodereview.appspot.com/6741002/diff/20001/src/Driver.h
File src/Driver.h (right):

https://rccodereview.appspot.com/6741002/diff/20001/src/Driver.h#newcode358
src/Driver.h:358: virtual void sendPacket(const Address *recipient,
Our coding standard is that the * goes next to the type (e.g., "Address*
recipient") not next to the variable. This actually makes sense, since the "*"
is part of the type. Can you back out all your changes where you moved it (both
here and in other files as well)?

https://rccodereview.appspot.com/6741002/diff/20001/src/Driver.h#newcode361
src/Driver.h:361: Buffer::Iterator *payload,
Here's a case where the star was on the wrong side all along. Please change to
"Buffer::Iterator*".

https://rccodereview.appspot.com/6741002/diff/20001/src/UdpDriver.cc
File src/UdpDriver.cc (right):

https://rccodereview.appspot.com/6741002/diff/20001/src/UdpDriver.cc#newcode79
src/UdpDriver.cc:79: }
Maybe the caller should drop everything up through the "+", so that we don't
need for every driver to individually do this? It seems to me that the locator
returned by the driver should be in a syntax matching the one that it received
in the constructor. Let's discuss.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld aab5469