| Message ID | 1457968700-23125-8-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Mar 14, 2016 at 06:18:17PM +0300, Ilya Maximets wrote: > This is unused functionality and it will harm to > multiqueue implementation. > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> This change doesn't make any sense to me. An equivalent change to the Linux kernel would mean that if two copies of "tcpdump -i eth0" were running then only one of them would get any packets.
On 22/03/2016 17:57, "Ben Pfaff" <blp@ovn.org> wrote: >On Mon, Mar 14, 2016 at 06:18:17PM +0300, Ilya Maximets wrote: >> This is unused functionality and it will harm to >> multiqueue implementation. >> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >This change doesn't make any sense to me. An equivalent change to the >Linux kernel would mean that if two copies of "tcpdump -i eth0" were >running then only one of them would get any packets. I independently decided to make a very similar code change with the same semantics to netdev-dummy, so maybe there's something I don't understand as well :-) netdev-dummy is like a physical NIC, the wire is a stream object (or the netdev-dummy/receive appctl, but only in one direction). When a multi queue NIC receives a packet from the wire it should put it in just one of its rings, right?
On Wed, Mar 23, 2016 at 01:13:06AM +0000, Daniele Di Proietto wrote: > > On 22/03/2016 17:57, "Ben Pfaff" <blp@ovn.org> wrote: > > >On Mon, Mar 14, 2016 at 06:18:17PM +0300, Ilya Maximets wrote: > >> This is unused functionality and it will harm to > >> multiqueue implementation. > >> > >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > >This change doesn't make any sense to me. An equivalent change to the > >Linux kernel would mean that if two copies of "tcpdump -i eth0" were > >running then only one of them would get any packets. > > I independently decided to make a very similar code change with the > same semantics to netdev-dummy, so maybe there's something I don't > understand as well :-) > > netdev-dummy is like a physical NIC, the wire is a stream object > (or the netdev-dummy/receive appctl, but only in one direction). > > When a multi queue NIC receives a packet from the wire it should > put it in just one of its rings, right? I don't understand the confusion. Suppose I call netdev_rxq_open() on a netdev that's backed by netdev-linux, say for eth0. That calls into netdev_linux_rxq_construct(), which opens a Linux AF_PACKET socket. From then on, I can call netdev_rxq_recv() on the netdev_rxq that I got (call it rxq0), and get a copy of each packet received on eth0. Suppose I call netdev_rxq_open() on the same netdev again. I get an independent netdev_rxq socket for eth0 (call it rxq1). I can call netdev_rxq_recv() on both rxq0 and rxq1 and I'll get two copies of the same stream of packets. netdev-dummy is implementing the same thing. struct netdev_dummy has a list of open "netdev_rxq"s as its member named 'rxes'. Each of those gets a copy of all the received packets. So...am I missing something?
On 22/03/2016 18:38, "Ben Pfaff" <blp@ovn.org> wrote: >On Wed, Mar 23, 2016 at 01:13:06AM +0000, Daniele Di Proietto wrote: >> >> On 22/03/2016 17:57, "Ben Pfaff" <blp@ovn.org> wrote: >> >> >On Mon, Mar 14, 2016 at 06:18:17PM +0300, Ilya Maximets wrote: >> >> This is unused functionality and it will harm to >> >> multiqueue implementation. >> >> >> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> > >> >This change doesn't make any sense to me. An equivalent change to the >> >Linux kernel would mean that if two copies of "tcpdump -i eth0" were >> >running then only one of them would get any packets. >> >> I independently decided to make a very similar code change with the >> same semantics to netdev-dummy, so maybe there's something I don't >> understand as well :-) >> >> netdev-dummy is like a physical NIC, the wire is a stream object >> (or the netdev-dummy/receive appctl, but only in one direction). >> >> When a multi queue NIC receives a packet from the wire it should >> put it in just one of its rings, right? > >I don't understand the confusion. > >Suppose I call netdev_rxq_open() on a netdev that's backed by >netdev-linux, say for eth0. That calls into >netdev_linux_rxq_construct(), which opens a Linux AF_PACKET socket. >From then on, I can call netdev_rxq_recv() on the netdev_rxq that I got >(call it rxq0), and get a copy of each packet received on eth0. > >Suppose I call netdev_rxq_open() on the same netdev again. I get an >independent netdev_rxq socket for eth0 (call it rxq1). I can call >netdev_rxq_recv() on both rxq0 and rxq1 and I'll get two copies of the >same stream of packets. > >netdev-dummy is implementing the same thing. struct netdev_dummy has a >list of open "netdev_rxq"s as its member named 'rxes'. Each of those >gets a copy of all the received packets. > >So...am I missing something? Oh, I see, I didn't consider this use case at all. Currently netdev-dpdk doesn't support this and it assumes that there will be only one open rxq per queue_id. The only user of rxqs is dpif-netdev and it is just opening one rxq per queue_id. I'm not sure it's necessary to support multiple receivers for the same packets, but I'm open to suggestions. I guess, at least, I'll need to document the current interface better. What do you think?
On Wed, Mar 23, 2016 at 07:27:08AM +0000, Daniele Di Proietto wrote: > > > On 22/03/2016 18:38, "Ben Pfaff" <blp@ovn.org> wrote: > > >On Wed, Mar 23, 2016 at 01:13:06AM +0000, Daniele Di Proietto wrote: > >> > >> On 22/03/2016 17:57, "Ben Pfaff" <blp@ovn.org> wrote: > >> > >> >On Mon, Mar 14, 2016 at 06:18:17PM +0300, Ilya Maximets wrote: > >> >> This is unused functionality and it will harm to > >> >> multiqueue implementation. > >> >> > >> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >> > > >> >This change doesn't make any sense to me. An equivalent change to the > >> >Linux kernel would mean that if two copies of "tcpdump -i eth0" were > >> >running then only one of them would get any packets. > >> > >> I independently decided to make a very similar code change with the > >> same semantics to netdev-dummy, so maybe there's something I don't > >> understand as well :-) > >> > >> netdev-dummy is like a physical NIC, the wire is a stream object > >> (or the netdev-dummy/receive appctl, but only in one direction). > >> > >> When a multi queue NIC receives a packet from the wire it should > >> put it in just one of its rings, right? > > > >I don't understand the confusion. > > > >Suppose I call netdev_rxq_open() on a netdev that's backed by > >netdev-linux, say for eth0. That calls into > >netdev_linux_rxq_construct(), which opens a Linux AF_PACKET socket. > >From then on, I can call netdev_rxq_recv() on the netdev_rxq that I got > >(call it rxq0), and get a copy of each packet received on eth0. > > > >Suppose I call netdev_rxq_open() on the same netdev again. I get an > >independent netdev_rxq socket for eth0 (call it rxq1). I can call > >netdev_rxq_recv() on both rxq0 and rxq1 and I'll get two copies of the > >same stream of packets. > > > >netdev-dummy is implementing the same thing. struct netdev_dummy has a > >list of open "netdev_rxq"s as its member named 'rxes'. Each of those > >gets a copy of all the received packets. > > > >So...am I missing something? > > Oh, I see, I didn't consider this use case at all. > > Currently netdev-dpdk doesn't support this and it assumes that there will > be only one open rxq per queue_id. > > The only user of rxqs is dpif-netdev and it is just opening one rxq per > queue_id. > > I'm not sure it's necessary to support multiple receivers for the same > packets, but I'm open to suggestions. I guess, at least, I'll need to > document the current interface better. > > What do you think? I guess if netdev-dpdk only supports one rxq per queue_id, that's fine; it can just return an error on an attempt to open the second one. If it's difficult to maintain the multiple-open behavior for netdev-dummy, then it could do the same thing. It would even simplify the code a little, since the list of rxqs could go away. The main thing I object to is how this patch allows multiple openers but then ignores all of them but one! Thanks, Ben.
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 0e536e8..4903e56 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -1338,26 +1338,22 @@ static void netdev_dummy_queue_packet(struct netdev_dummy *dummy, struct dp_packet *packet) OVS_REQUIRES(dummy->mutex) { - struct netdev_rxq_dummy *rx, *prev; + struct netdev_rxq_dummy *rx; if (dummy->rxq_pcap) { ovs_pcap_write(dummy->rxq_pcap, packet); fflush(dummy->rxq_pcap); } - prev = NULL; - LIST_FOR_EACH (rx, node, &dummy->rxes) { + + if (list_size(&dummy->rxes)) { + ASSIGN_CONTAINER(rx, list_front(&dummy->rxes), node); if (rx->recv_queue_len < NETDEV_DUMMY_MAX_QUEUE) { - if (prev) { - netdev_dummy_queue_packet__(prev, dp_packet_clone(packet)); - } - prev = rx; + netdev_dummy_queue_packet__(rx, packet); + return; } } - if (prev) { - netdev_dummy_queue_packet__(prev, packet); - } else { - dp_packet_delete(packet); - } + + dp_packet_delete(packet); } static void
This is unused functionality and it will harm to multiqueue implementation. Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/netdev-dummy.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)