diff mbox series

[v3,05/19] vhost: fix error handling when mem table gets updated

Message ID 20181004081403.8039-6-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: add postcopy live-migration support |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Maxime Coquelin Oct. 4, 2018, 8:13 a.m. UTC 
  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@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Ilya Maximets Oct. 4, 2018, 2:59 p.m. UTC | #1 
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@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---

Acked-by: Ilya Maximets <i.maximets@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)
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.

>  
>  			*pdev = dev;
>  		}
>
Maxime Coquelin Oct. 4, 2018, 3:06 p.m. UTC | #2 
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@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
> 
> Acked-by: Ilya Maximets <i.maximets@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.

[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;
>>   		}
>>
Ilya Maximets Oct. 4, 2018, 3:11 p.m. UTC | #3 
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@dpdk.org
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>
>> Acked-by: Ilya Maximets <i.maximets@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;
>>>           }
>>>
> 
>
diff mbox series

Patch

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;
+
 
 			*pdev = dev;
 		}