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? -boris
