Hi Ilya, Thanks for the comments, I have some responses in-line. I'll cook up the fixes for the next iteration, but would be interested in getting on your views, mainly on 1., 2. and 3..
On 30/08/2018 12:06, Ilya Maximets wrote: > Hi, Tiago. I applied that whole patch-set and looked through the > code. Didn't finish review yet, but have a few concerns about current > implementation: > > 1. dp_packet_l3/l4/others returns the pointer and checks only that > offset is less than the size of the first segment. But this check > doesn't guarantee that even one byte of this header located in > the first segment. Users will commonly read/write using that > pointer and crash/read wrong memory regions. For packets with small / few headers, there won't be any problems. But for packets where there's the need to access bigger / more headers, that's why dp_packet_linear_ofs() has been introduced. It copies the data so it can be accessed contiguously. More generally, the idea of the new linear APIs is - when a caller knows it will need all the data, such as a calculating a checksum or calling a write() it will need to use the linear APIs, instead of the the normal APIs. > > 2. mbuf modifications with buf_addr, data_off, etc changes > looks very dangerous. At least, in your current implementation > dp_packet_get_allocated() will return wrong value for reallocated > this way packet, because you're not updating nb_segs. > This is really hard to track all the mbuf fields that should be stored > and every DPDK upgrade may introduce very complex issues. > Maybe it's better to just clone the packet in a usual way? > > 3. dp_packet_linear_ofs(..) must be called before any call of > dp_packet_l3/l4/others ? Otherwise you will get a wrong pointer. > This is a bad concept from the API point of view. > Also, some users in your current implementation uses both > dp_packet_linear_ofs(pkt, pkt->l3_ofs) and dp_packet_l3(pkt) > at the same time for unknown reason. See handle_ftp_ctl() for > example. I see your point; The idea here was avoiding changing the callers of the dp_packet API as much as possible. If we make dp_packet_linear_data() call dp_packet_clone() then the caller will have to to update its pointer to the packet. In some cases this pointer is even `CONST *`. Hence why I opted for this approach. > > 4. handle_ftp_ctl() copies the whole packet even if function will > return after the first simple check.> > 5. You missed copying of 'rss_hash_valid' in dp_packet_clone_with_headroom(). > In general, there are a lot of code duplications which is > not a good thing in so complicated code. I don't understand this one; `rss_hash_valid` is only supposed to be used when OvS has been compiled without DPDK. With regards to the duplication, a previous review pointed out that there were two many compilation flags around dp_packet_clone_with_headroom(), hence why it has been broken out into two different implementations (which leads to some duplication). > > 6. No need to have netdev_dpdk_is_multi_segment_mbufs_enabled() in > a common header. > > 7. dp_packet_reset_packet() is broken. > dp_packet_set_size() should not change the real packet, i.e. free > the segments. In your implementation dp_packet_set_size() crops > the packet from the tail and after that dp_packet_set_data() > moves the head forward and reduces packet size again. As a result > dp_packet_reset_packet() eats the requested number of bytes from > both sides of the packet. You're right, I'll fix 6 and 7. However, dp_packet_size() was implemented like this (frees the tail segments) so that the tail of the packet is always in the last segment mbuf. This is to avoid returning a pointer to data that's segment in dp_packet_tail() or dp_packet_end(). > > 8. dp_packet_l3/l4/others returns NULL in case of headers placement > not in a first segment. But callers never checks the result. > They expect that these functions always return valid pointer > for the packets that contains these headers. In reality, > you can not be sure that all the headers are in the first segment. > For example in case of few levels of tunnelling. It is true that some places are not checking for NULL, but I believe I've covered most cases by using dp_packet_linear_ofs(). Are you talking of any case in specific? But given this is hidden behind the "dpdk-multi-seg-mbufs" flags I'd prefer to treat it on a case by case scenario other than "polluting" the code with NULL checks where none is required (most of the cases, I would say, won't require it). Thanks, Tiago. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
