Intel10g: rx_buffsize config and check

[javierguerragiraldez]

This is supposed to fix #272

• driver: The config parameter rx_buffsize is used to set the receive queue buffer size (rounded down to KB increments and clipped to 16KB max)
• driver: Before adding a buffer to the descriptor ring rxdesc, the size is assert()'ed to be big enough.
• intel_app: Before accepting a given rx_freelist, each buffer in it is checked for minimum size.

I don't know if the virtio app can switch from zero-copy to non-optimized at any time, so i opted for checking the whole freelist immediately, and ignoring it if it's not acceptable.

[lukego]

There is a problem with checking the freelist at init time: the freelist can be populated with different buffers over time so even if the contents were OK to start with this does not guarantee they will be OK in the future. (And in practice the freelist is probably empty at initialisation time and later becomes populated when buffers are retrieved from VMs.)

Good news though. The virtio app should be happy to accept both zero-copy and non-zero-copy buffers (or even a mix for different iovecs of the same packet). So I think it would work to initially accept the rx_freelist and then stop using it if we are ever given an unsuitable buffer.

Can we rename rx_buffsize to rx_buffersize? Otherwise I know I am going to confuse abbreviations buf vs buff sooner or later and when in doubt I think it's best not to abbreviate.

Is the rx_buffsize now a mandatory argument when creating the intel_app? (Have to update the code base for this? The SnabbBot CI test is failing on lib.nfv.config case -- is this the reason? does make test work for you?)

[javierguerragiraldez]

ok, i'm going to change the test from init to add_receive_buffers(). rx_buffersize isn't mandatory, at the moment it defaults to 4KB (the previous hardcoded value). The failure on lib.nfv.config was because i forgot the -1 on the testing loop.

(i'm updating with those changes right now)

[lukego]

⚠️ NEW IDEA WARNING :-) ⚠️

How about if intel10g automatically learns the minimum buffer size being used and reprograms the SRRCTL register for each hardware queue when needed?

One simple scheme could be to start with the maximum buffer size (16KB) and then for add_receive_buffer to detect when a receive buffer is too small and automatically reprogram SRRCTL with a safe value before adding the receive descriptor.

Then intel_app should still detect buffers that are too small and drop the freelist, but only for the extreme case when a buffer is so small that it can never be used by the NIC i.e. < 1KB.

What do you think? 👍 or 👎? (Sorry never tried Github emoticons before...)

[javierguerragiraldez]

ok, now add_receive_buffer dynamically adapts to given buffer size. intel_app replaces any buffer smaller than 1024 with a generic one.
I'm not sure what to do with those "too small" buffers. now i'm just disposing them because buffer.free() seems intelligent enough. but what would happen to the just allocated buffer when the packet is deref()ed? will it return to the generic freelist? if so, the next intel_app:add_receive_buffers() would add less buffers, potentially zero...

[lukego]

This behaviour seems reasonable to me for dealing with too-small buffers.

[eugeneia]

This PR breaks lib/nfv/selftest.sh:

Restarting Virtio_B (died at 1413207491.867421: lib/virtio/net_device.lua:220: attempt to index local 'b' (a nil value))

SnabbBot didn't catch it because lib/nfv/selftest.sh didn't exist back then. Can we revert and reopen this one?

[javierguerragiraldez]

hum... that part of virtio/net_device.lua copies packets without first checking if it could allocate the destination. should it be fixed on this PR?

(related question: if reverted, how can it be reopened? last time i had to do a bit of careful cherry-picking)

[eugeneia]

I didn't know how long it would take to find the bug that's why I suggested reverting this PR for the time being but I don't mind how the fix gets into master. Just make sure master is "broken" as short as possible.

[lukego]

Will pull req 286 fix this?

#286

On Monday, October 13, 2014, Max Rottenkolber notifications@github.com
wrote:

I didn't know how long it would take to find the bug that's why I
suggested reverting this PR for the time being but I don't mind how the
fix gets into master. Just make sure master is "broken" as short as
possible.

—
Reply to this email directly or view it on GitHub
#274 (comment).

[eugeneia]

Yes #286 did fix it!