Fixing retrieval of original mbuf in ToDPDKDevice.

[davidek]

Closes #6

[davidek]

With respect to the original patch, I added a defensive assertion so that if (it is compiled with assertions enabled and) the assumption turns out to be wrong it doesnt segfault.

[tbarbette]

Thought of a problem just in time... If the packet is shared and we have the clone, the destructor argument is not set, only the destructor argument of the p->data_packet()...

Is using mbuf = (struct rte_mbuf *) (p->buffer() - sizeof(struct rte_mbuf)); really a problem?

It should be constant...

[davidek]

I also thought of this possibility, but it appears to me the current implementation of clone() copies the _destructor_argument in the memcpy operation and does not overwrite it (why should it?).
Nevertheless, it might be worth to check if (!mbuf && p->data_packet()) {...}. Do you want me to ad it to the PR?

Yes, (p->buffer() - sizeof(struct rte_mbuf)) caused bad things to happen on my system (my best bet would be that the difference is in the dpdk version? I'm using 2.2.0): if you look at the output of the debug prints I copied over in #6 you'll see that the addresses are indeed different on my instance (which in turn caused wrong packets to be sent around by dpdk...)

[tbarbette]

Mhh yes, only the _destructor is resetted to 0 in clone() just after the memcpy, I thought it was both...

That's at least the less unclean for now.

Thanks !

[tbarbette]

No the assert is enough, if not too much already... This is in a very fast path, called for every packet. At 40G every instruction counts... We should just think it through, rely on tests, and have the fewer runtime test as possible....

[davidek]

Indeed, I figured copiling for performance would require -DNDEBUG? (Perhaps a line in the README mentioning this would be worth?)

[tbarbette]

Never used it. Does it change a lot? Disabling all assert is not a good idea either. Some are needed to prevent mis-configuration from the user. In our present case, I meant it is possible to completely think of all cases, go through all possible changes of in _destructor_argument, and be sure that the code is correct and we can rely on destructor_argument() without asserting after.

But there is some assert in some place which check for example that the ip header is set, and that only relies on the user having placed a MarkIPHeader or CheckIPHeader before...