[dpdk-dev] [PATCH v3 05/19] vhost: fix error handling when mem table gets updated

Ilya Maximets i.maximets at samsung.com
Thu Oct 4 17:11:54 CEST 2018


On 04.10.2018 18:06, Maxime Coquelin wrote:
> 
> 
> On 10/04/2018 04:59 PM, Ilya Maximets wrote:
>> On 04.10.2018 11:13, Maxime Coquelin wrote:
>>> When the memory table gets updated, the rings addresses need
>>> to be translated again. If it fails, we need to exit cleanly
>>> by unmapping memory regions.
>>>
>>> Fixes: d5022533c20a ("vhost: retranslate vring addr when memory table changes")
>>> Cc: stable at dpdk.org
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin at redhat.com>
>>> ---
>>
>> Acked-by: Ilya Maximets <i.maximets at samsung.com>
>>
>> Minor comments inline.
>>
>>>   lib/librte_vhost/vhost_user.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 8ffe5aa66..b6eae8dc5 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -964,7 +964,8 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg)
>>>                 dev = translate_ring_addresses(dev, i);
>>>               if (!dev)
>>> -                return VH_RESULT_ERR;
>>> +                goto err_mmap;
>>> +
>>
>> 1. No need to have two empty lines. (You could fix this while applying)
> 
> Right. I will either fix it while applying, or it will be in next
> revision, if any.
> 
>> 2. In current code, error on message handling will cause disconnect and
>>     memory regions will be freed anyway. So, the change is not very
>>     important for master (maybe just for consistency with surrounding
>>     code) but it could be important for stable versions.
> 
> I may not disconnect directly, as if reply-ack feature is negotiated[0],
> the slave would reply with a NACK and function handler will return 0.
> I guess that in that case the master (QEMU) will disconnect anyway, but
> this is just an assumption from slave point of view.

Yes, you're right. Thanks.

> 
> [0]: REPLY_ACK protocol feature is only advertised when IOMMU support is
> requested because of a bug in an old upstream QEMU version.
> 
> Thanks!
> Maxime
> 
>>>                 *pdev = dev;
>>>           }
>>>
> 
> 


More information about the dev mailing list