Driver changes

Driver changes

[yilongl, 2017-02-06 23:23:27]

This patch does two major things:

* Add priority to the Driver interface
* Actually use priority in DpdkDriver

[yilongl, 2017-02-07 05:15:58]

On 2017/02/06 23:23:27, yilongl wrote:
> This patch does two major things:
> 
> * Add priority to the Driver interface
> * Actually use priority in DpdkDriver

OK, it turns out I am not done yet. Currently, DpdkDriver has no unit test at
all. I am not very comfortable with it so I am going to write a few unit tests.
I think there is no need to review this patch until then.

Thanks,
Yilong

[ouster, 2017-02-07 16:52:52]

OK; I'll wait.

-John-

On Mon, Feb 6, 2017 at 9:15 PM, <yilongl@stanford.edu> wrote:

> On 2017/02/06 23:23:27, yilongl wrote:
>
>> This patch does two major things:
>>
>
> * Add priority to the Driver interface
>> * Actually use priority in DpdkDriver
>>
>
> OK, it turns out I am not done yet. Currently, DpdkDriver has no unit
> test at all. I am not very comfortable with it so I am going to write a
> few unit tests. I think there is no need to review this patch until
> then.
>
> Thanks,
> Yilong
>
> https://rccodereview.appspot.com/6741002/
>

[yilongl, 2017-02-08 22:00:38]

I have uploaded a second patch set. This is now ready for review.

Thanks,
Yilong

On 2017/02/07 16:52:52, ouster wrote:
> OK; I'll wait.
> 
> -John-
> 
> On Mon, Feb 6, 2017 at 9:15 PM, <mailto:yilongl@stanford.edu> wrote:
> 
> > On 2017/02/06 23:23:27, yilongl wrote:
> >
> >> This patch does two major things:
> >>
> >
> > * Add priority to the Driver interface
> >> * Actually use priority in DpdkDriver
> >>
> >
> > OK, it turns out I am not done yet. Currently, DpdkDriver has no unit
> > test at all. I am not very comfortable with it so I am going to write a
> > few unit tests. I think there is no need to review this patch until
> > then.
> >
> > Thanks,
> > Yilong
> >
> > https://rccodereview.appspot.com/6741002/
> >

[ouster, 2017-02-14 22:58:47]

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.