Redeclaration of 'should_continue' is hidding external variable

[dpino]

Redeclaration of variable 'should_continue' in the inner loop is hiding value of external variable 'should_continue'.

We have doubts this is the intended behaviour. It seems 'should_continue' is used to keep track of whether exit the outer loop. Apparently this change was introduced in d62ffc1 (net_device: Refactor vrings).

[lukego]

This code is sensitive because it's the main performance hotspot for the NFV application. Changes here need to be checked for performance implications on the "NFV loadgen benchmark" before we can merge them. @eugeneia has a new version of the benchmark (#307) that I will merge now. Then we can get the key benchmark running nicely on chur and perhaps hook it into the CI too.

[dpino]

I managed to run the "NFV loadgen benchmark" in grindewald, with and without the patch applied. Here are the results:

Patch not applied

Connect to guests with:
telnet localhost 5500
telnet localhost 5501
telnet localhost 5502
telnet localhost 5503
On 0000:05:00.0 got 3.357
On 0000:82:00.0 got 3.226
On 0000:84:00.0 got 3.194
On 0000:88:00.0 got 3.196
Rate(Mpps): 12.973

Patch applied

Connect to guests with:
telnet localhost 5500
telnet localhost 5501
telnet localhost 5502
telnet localhost 5503
On 0000:05:00.0 got 3.342
On 0000:82:00.0 got 3.250
On 0000:84:00.0 got 3.194
On 0000:88:00.0 got 3.205
Rate(Mpps): 12.991

So it seems it doesn't make a difference. I would let @N-Nikolaev decide whether this logic is correct or not. Summarizing, in d62ffc1 (net_device: Refactor vrings) variable should_continue was redeclared within the for loop in VirtioNetDevice:transmit_packets_to_vm. Since the variable within the for loop is different than the more external should_continue, this external variable is never updated.

[ghost]

This is definitely a bug.
Just out of curiosity can it be written like this:
should_continue, local b = self:vm_buffer(iovec)

[dpino]

As far as I know, cannot. When trying to compile a statement like that LuaJIT returns error "unexpected symbol near 'local'".

Before applying the patch, I'd prefer to run the "NFV loadgen benchmark" in chur and check there are no regressions. What's the current the status of this?

[lukego]

@dpino You already ran the loadgen benchmark on Grindelwald and found that it does not affect performance, right?

[dpino]

@lukego Yes, that's correct, I got similar results #315 (comment)

[lukego]

Thanks for the patch!