Driver clean-ups

Driver clean-ups

[yilongl, 2016-08-03 23:20:13]

Hi John,

I am doing some small clean-ups while reading the code to help me understand the
code. I see that you are working on the InfUdDriver so you can ignore my minor
changes to that class. Thanks!

commit 27d03c104e29bc91950a8a9c7ef94ad60a5fc88a
Author: Yilong Li <yilongl@cs.stanford.edu>
Date:   Wed Aug 3 15:32:06 2016 -0700

    Multiple clean-ups for the driver classes.
    
    * Pulled PacketBuf up into a class template in Driver
    * Removed the virtual clone function in Driver::Address, which is only used
      in the unit tests; redesigned the unit tests such that the PacketBuf's are
      only returned to the object pool when they become useless
    * Removed unused functions, definitions, etc.
    * Added const modifiers in Driver
    * Fixed typos in documentation

[ouster, 2016-08-04 16:14:40]

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
src/DpdkDriver.h:70: typedef Driver::PacketBuf<MacAddress, MAX_PAYLOAD_SIZE>
PacketBuf;
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).

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;
Why are these lines not needed? Let's discuss.

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:
protected => PROTECTED

(For testing; I can explain if you haven't seen this before)

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!?
Until recently, none of the drivers have supported zero copy. I'm in the process
of implementing it for InfUdDriver.

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).
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).

[yilongl, 2016-08-04 23:08:13]

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.