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
