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

Issue 8771002: Driver clean-ups

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 5 months ago by yilongl
Modified:
5 years, 5 months ago
Reviewers:
ouster
Visibility:
Public.

Description

Driver clean-ups

Patch Set 1 #

Total comments: 10

Patch Set 2 : fix DpdkDriver and revert InfUdDriver #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -19 lines) Patch
M .gitignore View 1 1 chunk +1 line, -0 lines 0 comments Download
M bindings/java/build.gradle View 1 2 chunks +2 lines, -2 lines 0 comments Download
M clientTests/RAM-462/Makefile View 1 1 chunk +2 lines, -2 lines 0 comments Download
M scripts/dpdkBuild.sh View 1 1 chunk +2 lines, -1 line 0 comments Download
M src/DpdkDriver.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/Driver.h View 1 2 chunks +1 line, -2 lines 0 comments Download
M src/InfUdDriver.h View 1 1 chunk +13 lines, -7 lines 0 comments Download
M src/InfUdDriver.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/MakefragTest View 1 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 3
yilongl
Hi John, I am doing some small clean-ups while reading the code to help me ...
5 years, 5 months ago (2016-08-03 23:20:13 UTC) #1
ouster
These cleanups look good; just a couple of small comments. https://rccodereview.appspot.com/8771002/diff/1/src/DpdkDriver.h File src/DpdkDriver.h (right): https://rccodereview.appspot.com/8771002/diff/1/src/DpdkDriver.h#newcode70 ...
5 years, 5 months ago (2016-08-04 16:14:40 UTC) #2
yilongl
5 years, 5 months ago (2016-08-04 23:08:13 UTC) #3
I have fixed the dpdkBuild script, tested DpdkDriver, reverted changes to
InfUdDriver, and resolved the rest of the small comments.

The second patch set is now uploaded.

https://rccodereview.appspot.com/8771002/diff/1/src/DpdkDriver.h
File src/DpdkDriver.h (right):

https://rccodereview.appspot.com/8771002/diff/1/src/DpdkDriver.h#newcode70
src/DpdkDriver.h:70: typedef Driver::PacketBuf<MacAddress, MAX_PAYLOAD_SIZE>
PacketBuf;
On 2016/08/04 16:14:40, ouster wrote:
> Have you tried compiling DpdkDriver to make sure this all works? By default,
the
> DPDK support isn't compiled in RAMCloud (you have to set a variable in
> GNUmakefile).

Done.

https://rccodereview.appspot.com/8771002/diff/1/src/Driver.h
File src/Driver.h (left):

https://rccodereview.appspot.com/8771002/diff/1/src/Driver.h#oldcode105
src/Driver.h:105: other.driver = NULL;
On 2016/08/04 16:14:40, ouster wrote:
> Why are these lines not needed? Let's discuss.

I made these two fields const so they couldn't be modified. OTOH, payload alone
is sufficient to indicate whether this Received is valid.

https://rccodereview.appspot.com/8771002/diff/1/src/Driver.h
File src/Driver.h (right):

https://rccodereview.appspot.com/8771002/diff/1/src/Driver.h#newcode57
src/Driver.h:57: protected:
On 2016/08/04 16:14:40, ouster wrote:
> protected => PROTECTED
> 
> (For testing; I can explain if you haven't seen this before)

Done.

https://rccodereview.appspot.com/8771002/diff/1/src/Driver.h#newcode305
src/Driver.h:305: // TODO(YilongL): why doesn't any subclass implement this
method!?
On 2016/08/04 16:14:40, ouster wrote:
> Until recently, none of the drivers have supported zero copy. I'm in the
process
> of implementing it for InfUdDriver.

Acknowledged.

https://rccodereview.appspot.com/8771002/diff/1/src/InfUdDriver.h
File src/InfUdDriver.h (right):

https://rccodereview.appspot.com/8771002/diff/1/src/InfUdDriver.h#newcode103
src/InfUdDriver.h:103: /// Address of sender (if we are in Ethernet mode).
On 2016/08/04 16:14:40, ouster wrote:
> In the new version of InfUdDriver that I'm working on, this class has gone
away
> completely (this is only needed if input packets get copied).

Acknowledged.
Sign in to reply to this message.

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