Hi Anju, On 10/05/2018 09:51, Anju Thomas wrote:
[snip] > -----Original Message----- > From: Ben Pfaff [mailto:[email protected]] > Sent: Thursday, May 10, 2018 1:59 AM > To: Anju Thomas <[email protected]> > Cc: [email protected] > Subject: Re: [ovs-dev] [PATCH v1] Fix crash due to multiple tnl push action > > Thanks for the patch. It applies OK and all the tests pass. > > It looks like your commit message describes at least two other bugs in OVS, > though. First, if OVS crashes when it pushes tunnel headers, even if there's > not enough headroom, that's really bad. At worst, it should drop the packet. > Do you know where the crash occurs? We should fix the problem, since it > might recur in some other context. > > [Anju] OVS was actually crashing in the dpdk datapath. The crash is a manual > assert in our case. > The rootcause is that dp receives the actions after the upcall (say with >=3 > tunnel pushes ) . Now as part of action processing , since it is a tunnel > push action , we try to find space in the dpdk mbuf packet headroom (which Is > 128 bytes). By the time we try to process the third tunnel push , there is no > headroom left since each tunnel header is of 50 bytes (50 *3 > 128 bytes > headroom). Hence it manually asserts . This assert is to catch any > unexpected code flow . Do you think that in this case we should still go > ahead and prevent the crash ? > > The back trace was as below:- > (gdb) bt > #0 0x00007ffa9a856c37 in __GI_raise (sig=sig@entry=6) at > ../nptl/sysdeps/unix/sysv/linux/raise.c:56 > #1 0x00007ffa9a85a028 in __GI_abort () at abort.c:89 > #2 0x00000000005fc725 in dp_packet_resize__ (b=b@entry=0x7ffa87744680, > new_headroom=new_headroom@entry=64, new_tailroom=<optimized out>) > at lib/dp-packet.c:258 > #3 0x00000000005fcb1f in dp_packet_prealloc_headroom > (b=b@entry=0x7ffa87744680, size=size@entry=50) at lib/dp-packet.c:288 > #4 0x00000000005fcf91 in dp_packet_push_uninit (b=b@entry=0x7ffa87744680, > size=size@entry=50) at lib/dp-packet.c:400 > #5 0x000000000069466c in netdev_tnl_push_ip_header > (packet=packet@entry=0x7ffa87744680, header=0x7ff85401bab0, size=50, > ip_tot_size=ip_tot_size@entry=0x7ff8117f89fc) at > lib/netdev-native-tnl.c:152 > #6 0x000000000069472a in netdev_tnl_push_udp_header (packet=0x7ffa87744680, > data=<optimized out>) at lib/netdev-native-tnl.c:215 > #7 0x0000000000625627 in netdev_push_header (netdev=0x3ca3990, > batch=batch@entry=0x7ff8117f9498, data=data@entry=0x7ff85401baa0) > at lib/netdev.c:874 > #8 0x00000000006069f2 in push_tnl_action (batch=0x7ff8117f9498, > attr=0x7ff85401ba9c, pmd=0x33efb30) at lib/dpif-netdev.c:4629 > #9 dp_execute_cb (aux_=aux_@entry=0x7ff8117f9840, > packets_=packets_@entry=0x7ff8117f9498, a=a@entry=0x7ff85401ba9c, > may_steal=false) > > > static void > dp_packet_resize__(struct dp_packet *b, size_t new_headroom, size_t > new_tailroom) > { > void *new_base, *new_data; > size_t new_allocated; > > new_allocated = new_headroom + dp_packet_size(b) + new_tailroom; > > switch (b->source) { > case DPBUF_DPDK: > > OVS_NOT_REACHED();-------------------------------------------------------------------------------------------------->crashes > here > Just to point out that we are working on adding support for dynamically allocating more mbufs, as part of our multi-segment mbufs series. In this case however, still using a single mbuf, I think there's two points worth noting: 1. Mbufs are allocated with the maximum possible packet size based on the MTU. And given that a single mbuf is used per packet, one can't (dynamically) allocate more memory to increase that limit. The solution for this is to append more mbufs, the multi-segment mbufs approach; 2. As you mention, typically the size set for the headroom in a mbuf is 128B. Point 2. doesn't seem to be enough for your use-case and, nonetheless, I agree that crashing OvS because there's not enough space, either headroom or tailroom, to accommodate for new data is not the best approach. I was planning to work on a proposal to deal with this for single mbufs as well, but I see you have already committed to that so I'll withdraw myself. I'll keep and eye out, though, and will gladly review / test once you submit. Thanks, Tiago. Note: One can still increase the headroom by building DPDK using the config option `CONFIG_RTE_PKTMBUF_HEADROOM`, which is set to 128B by default. However, this doesn't seem to be done often and I can not vouch for testing coverage here. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
