tweaking iterators?

[rbtcollins]

Seeking feedback on this idea.

Right now the SlotIter iterators on {R,T}xRing mutate the ring as they go - meaning that code like the following is broken:

       {
            let tx_slots = nm_in_host.tx_iter().flat_map(|tx_ring| tx_ring.iter());
            let rx_slots = nm_in.rx_iter().flat_map(|rx_ring| rx_ring.iter());
            for (tx_slot_buf, rx_slot_buf) in tx_slots.zip(rx_slots) {
                ... forward from rx_slot_buf to tx_slot_buf...
            }
        };

There are actually multiple defects there. Firstly, as soon as either ring is empty, we have either discarded a rx packet (because the tx ring was empty but next() was called on the rx ring) or secondly we've generated a totally bogus tx packet (because the rx ring was empty but next() was called on the tx ring).

Fixing that by not using zip retains the defect: which ever iterator we use we'll either end up discarding a packet we recieved, or generating a bogus packet to send.

Its not clear to me from reading the netmap headers and code that we can depend on tail never changing outside of syscalls.:

 * DATA OWNERSHIP/LOCKING:
 *      The netmap_ring, and all slots and buffers in the range
 *      [head .. tail-1] are owned by the user program;
 *      the kernel only accesses them during a netmap system call
 *      and in the user thread context.

There's no guaranteed about anything before head - so if we advance head we can't presume that we can reverse it.

I can think of a few ways to address this. One is to say 'hey the iterator interface is for simple use, sorry'. Another, which I favour, would be to not touch head, only cur - allowing folk to decrement cur to undo the last iterator's next(), and have a method to sync head to cur. The above code could then be safely written as

       {
            let tx_slots = nm_in_host.tx_iter().flat_map(|tx_ring| tx_ring.iter());
            let rx_slots = nm_in.rx_iter().flat_map(|rx_ring| rx_ring.iter());
            for tx_slot_buf in tx_slots {
                 match rx_slots.next() {
                     None => tx_slot_buf.give_back();
                     Some(rx_slot_buf) => ... copy from rx to tx
            }
            nm_in_host.tx_iter().map(|ring| ring.head_from_cur());
            nm_in_host.rx_iter().map(|ring| ring.head_from_cur());
        };

What do you think?

[rbtcollins]

Also, I've just realised we have a big lifetime issue here - we hand out a mutable slot reference, but once we've advanced head, thats unsafe to hold onto, so we probably want the lifetime to be much tighter - I'm looking into streaming iterators now which seem like they may address that.

[polachok]

Yes, I'm aware of this safety problem (that's one of the reasons I say the crate is totally unsafe in the README). For my use case (remember that it started as a copy-paste from my app) this is not a problem because of how app is designed, but if we want to make a general purpose library, this should be worked around somehow (and I don't know how).

[rbtcollins]

There's another related issue - you can't currently have an iterator over both the rx and tx rings; the underlying cause there is afaict the reference to the overall descriptor.

[polachok]

We have new_with_memory() and clone_ring() to avoid this problem.

[rbtcollins]

Ah thanks, I'll poke around that more closely. Sorry if I'm asking newbie questions too much - still learning rust's idioms and patterns.

[rbtcollins]

So I'm coming back - there doesn't seem to be any data race involved in using a single fd for both TX and RX, its just that the C dats structure isn't nicely partitioned - its a long vec of pointers split in two based on a counter. So from that we could use a readonly ref to ring_ofs + a readonly ref to ni_tx_rings and we can calculate any rx ring offset or tx ring offset without races, but we'd need to not use the NETMAP_TXRING macros (because they want the nifp base address. I'd like to avoid the spurious open, even though thats perhaps ocd of me :)

[polachok]

It's not a macro in rust, see https://github.com/libpnet/netmap_sys/blob/master/src/netmap_user.rs#L26

because they want the nifp base address. I'd like to avoid the spurious open, even though thats perhaps ocd of me

I don't quite see what you mean by "spurious open", can you elaborate?

[rbtcollins]

Oh, didn't realise cause it had the same name .): Anyhow, if we remodelled things a little we could have two vectors for the ring offsets - unless the kernel module can move those around at choice? AFAICT it cannot, it can only change the layout when an interface is configured, and then under hard constraints.

spurious open - I mean having to have a separate kernel fd for each direction of rings, since netmaps core doesn't need that.

[johalun]

@rbtcollins Hey! How did it go with the iterators? Did you implement any of the ideas you had?

[rbtcollins]

Yes, I can't remember if I put up a PR, but I've got a thing working that is, I think, correct, and able to do the packet forwarding by ring manipulation I desired. Probably in my github repo if I haven't put a PR up.

[johalun]

Ah I see, you simply made the tx iterator immutable but the inside buf mutable?

[rbtcollins]

I did do interior mutability yes; I'm not 100% sure that thats the best solution, but doing an ownership split on an iterator is a bit mind bending :). I'm not sure I've addressed the potential race I described in filing this bug on closer look - but my code works so far, so shrug :P