[ovs-dev] [PATCH v7 00/13] Support multi-segment mbufs

[Ian Stokes]

On 8/9/2018 12:44 PM, Ilya Maximets wrote:
> On 09.08.2018 11:38, Lam, Tiago wrote:
>> Hi Ilya,
>>
>> On 09/08/2018 09:27, Ilya Maximets wrote:
>>> Hmm. I found that this series modifies only dpdk related components
>>> and doesn't pay any attention to others.
>>> dp_packet API was modified to respect the segmented packets, but
>>> there are many places, where code uses packet data directly using
>>> only dp_packet_data() pointer and dp_packet_size().
>>> For example, at least following functions will read the wrong memory
>>> and probably generate segfault:
>>>
>>
>> No, that's not correct. Actually, this has been explained back in June
>> [1], where Eelco raised the same concerns. There, I replied with:
>>
>> "This series takes the approach of not allocating new mbufs in
>> `DPBUF_DPDK` dp_packets. This is to avoid the case where a
>> dp_packet_put() call, for example, allocates two mbufs to create enough
>> space, and returns a pointer to the data which isn't contiguous in
>> memory (because it is spread across two mbufs). Most of the code in OvS
>> that uses the dp_packet API relies on the assumption that memory
>> returned is contiguous in memory."
>>
>> But please do refer to the link for a full explanation. In fact, if that
>> was the case most of the tests in system-traffic would fail, and that
>> would show a flaw in the design.
>>
>> Now, obviously this doesn't mean that in the odd case something may have
>> slip through the cracks, but all the tests that have been run so far
>> have given enough confidence.
>>
>> Tiago.
>>
>> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2018-June/348502.html
> 
> I don't understand what you're talking about.
> Just look at the following case:
> 
> 1. Applying following patch, just to be sure:
> ---
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 0c42268..bc995bc 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1433,6 +1433,11 @@ netdev_linux_tap_batch_send(struct netdev *netdev_,
>           ssize_t retval;
>           int error;
>   
> +        if (packet->mbuf.nb_segs > 1) {
> +            VLOG_ERR_RL(&rl,
> +                        "Sending multiseg mbuf as a plain data. nb_segs: %d",
> +                        packet->mbuf.nb_segs);
> +        }
>           do {
>               retval = write(netdev->tap_fd, dp_packet_data(packet), size);
>               error = retval < 0 ? errno : 0;
> ---
> 
> 2. Configure OVS:
> ./bin/ovs-vsctl set Open_vSwitch . other_config:dpdk-multi-seg-mbufs=true
> 
> 3. Create one bridge with datapath_type "netdev", add one vhostuserclient
>     port "vhost1" like this:
> 
> # ovs-vsctl show
>      Bridge "ovsdpdk_br0"
>          Port "vhost1"
>              Interface "vhost1"
>                  type: dpdkvhostuserclient
>                  options: {vhost-server-path="/vhost1.sock"}
>          Port "ovsdpdk_br0"
>              Interface "ovsdpdk_br0"
>                  type: internal
> 
> 4. Start VM.
> 
> 5. Set up on host:
> 
> # ovs-vsctl set interface vhost1 mtu_request=9000
> # ovs-vsctl set interface ovsdpdk_br0 mtu_request=9000
> # ifconfig ovsdpdk_br0 192.168.114.20 netmask 255.255.255.0
> 
> 6. Set up inside VM:
> 
> # ifconfig eth0 192.168.114.10 netmask 255.255.255.0 mtu 9000
> 
> 7. Try to scp large file from VM to Host:
> 
> # scp file root at 192.168.114.20:./
> file                             13% 2112KB 434.8KB/s - *stalled*
> 
> At the same time in tcpdump on the host side:
> 
> 14:17:51.162168 IP (tos 0x8, ttl 64, id 30567, offset 0, flags [DF], proto TCP (6), length 9000)
>      192.168.114.10.43648 > 192.168.114.20.ssh: Flags [.], cksum 0xc831 (incorrect -> 0x1be9), seq 480:9428, ack 761, win 256, options [nop,nop,TS val 216598 ecr 1685821578], length 8948
> 14:17:51.582171 IP (tos 0x8, ttl 64, id 30568, offset 0, flags [DF], proto TCP (6), length 9000)
>      192.168.114.10.43648 > 192.168.114.20.ssh: Flags [.], cksum 0xc807 (incorrect -> 0x980f), seq 480:9428, ack 761, win 256, options [nop,nop,TS val 216640 ecr 1685821578], length 8948
> 14:17:52.422175 IP (tos 0x8, ttl 64, id 30569, offset 0, flags [DF], proto TCP (6), length 9000)
>      192.168.114.10.43648 > 192.168.114.20.ssh: Flags [.], cksum 0xc7b3 (incorrect -> 0x1b6b), seq 480:9428, ack 761, win 256, options [nop,nop,TS val 216724 ecr 1685821578], length 8948
> 14:17:54.115128 IP (tos 0x8, ttl 64, id 30570, offset 0, flags [DF], proto TCP (6), length 9000)
>      192.168.114.10.43648 > 192.168.114.20.ssh: Flags [.], cksum 0xc70b (incorrect -> 0x9713), seq 480:9428, ack 761, win 256, options [nop,nop,TS val 216892 ecr 1685821578], length 8948
> 
> And in OVS log:
> 
> 2018-08-09T11:17:50Z|00002|netdev_linux(pmd8)|ERR|Sending multiseg mbuf as a plain data. nb_segs: 4
> 2018-08-09T11:17:50Z|00003|netdev_linux(pmd8)|ERR|Sending multiseg mbuf as a plain data. nb_segs: 5
> 2018-08-09T11:17:50Z|00004|netdev_linux(pmd8)|ERR|Sending multiseg mbuf as a plain data. nb_segs: 4
> 2018-08-09T11:17:50Z|00005|netdev_linux(pmd8)|ERR|Sending multiseg mbuf as a plain data. nb_segs: 5
> 2018-08-09T11:17:50Z|00006|netdev_linux(pmd8)|ERR|Sending multiseg mbuf as a plain data. nb_segs: 5
> 2018-08-09T11:17:51Z|00007|netdev_linux(pmd8)|ERR|Sending multiseg mbuf as a plain data. nb_segs: 5
> 2018-08-09T11:17:51Z|00008|netdev_linux(pmd8)|ERR|Sending multiseg mbuf as a plain data. nb_segs: 5
> 
> SCP never finished because of corrupted data.
> Same test works fine if other_config:dpdk-multi-seg-mbufs=false.
> 
> Best regards, Ilya Maximets.
> 


I'm holding off merging this just yet, the questions around issues 
flagged here should be resolved first, there are also a number of sparse 
build issues with the series to be resolved so another revision at least 
is required to address these problems.

Ian