Re: [Xen-devel] [PATCH] xen-blkfront: use old rinfo after enomem during migration

[Juergen Gross <jgross@xxxxxxxx>]

On 04/12/2018 02:14, Dongli Zhang wrote:
> Hi Boris,
> 
> On 12/04/2018 12:07 AM, Boris Ostrovsky wrote:
>> On 12/2/18 3:31 PM, Manjunath Patil wrote:
>>> On 11/30/2018 2:33 PM, Boris Ostrovsky wrote:
>>>
>>>> On 11/30/18 4:49 PM, Manjunath Patil wrote:
>>>>> Thank you Boris for your comments. I removed faulty email of mine.
>>>>>
>>>>> replies inline.
>>>>> On 11/30/2018 12:42 PM, Boris Ostrovsky wrote:
>>>>>> On 11/29/18 12:17 AM, Manjunath Patil wrote:
>>>>>>> Hi,
>>>>>>> Feel free to suggest/comment on this.
>>>>>>>
>>>>>>> I am trying to do the following at dst during the migration now.
>>>>>>> 1. Dont clear the old rinfo in blkif_free(). Instead just clean it.
>>>>>>> 2. Store the old rinfo and nr_rings into temp variables in
>>>>>>> negotiate_mq()
>>>>>>> 3. let nr_rings get re-calculated based on backend data
>>>>>>> 4. try allocating new memory based on new nr_rings
>>>>>> Since I suspect number of rings will likely be the same why not reuse
>>>>>> the rings in the common case?
>>>>> I thought attaching devices will be more often than migration. Hence
>>>>> did not want add to an extra check for
>>>>>    - if I am inside migration code path and
>>>>>    - if new nr_rings is equal to old nr_rings or not
>>>>>
>>>>> Sure addition of such a thing would avoid the memory allocation
>>>>> altogether in migration path,
>>>>> but it would add a little overhead for normal device addition.
>>>>>
>>>>> Do you think its worth adding that change?
>>>>
>>>> IMO a couple of extra checks are not going to make much difference.
>>> I will add this change
>>>>
>>>> I wonder though --- have you actually seen the case where you did fail
>>>> allocation and changes provided in this patch made things work? I am
>>>> asking because right after negotiate_mq() we will call setup_blkring()
>>>> and it will want to allocate bunch of memory. A failure there is fatal
>>>> (to ring setup). So it seems to me that you will survive negotiate_mq()
>>>> but then will likely fail soon after.
>>> I have noticed the ENOMEM insise negotiate_mq() on ct machine. When I
>>> included my patch, I manually triggered the ENOMEM using a debug flag.
>>> The patch works for ENOMEM inside negotiate_mq().
>>>
>>> As you mentioned, if we really hit the ENOMEM in negotiate_mq(), we
>>> might hit it in setup_blkring() as well.
>>> We should add the similar change to blkif_sring struct as well.
>>
>>
>> Won't you have a similar issue with other frontends, say, netfront?
> 
> I think the kmalloc is failed not because of OOM.
> 
> In fact, the size of "blkfront_ring_info" is large. When domU have 4
> queues/rings, the size of 4 blkfront_ring_info can be about 300+ KB.
> 
> There is chance that kmalloc() 300+ KB would fail.

So kmalloc() might not be the best choice. Any reason why you don't
change it to vmalloc()? This should address the problem in a much
simpler way.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel