On 12/3/2018 6:16 PM, Boris Ostrovsky wrote:
On 12/3/18 8:14 PM, 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.
About netfront, to kmalloc() 8 'struct netfront_queue' seems consumes <70 KB?
TBH these look like comparable sizes to me. I am not convinced that
these changes will make a difference. If the number of rings on source
and destination were the same I'd absolutely agree with this patch but
since you are trying to handle different sizes the code becomes somewhat
more complex, and I am not sure it's worth it. (Can you actually give me
an example of when we can expect number of rings to change during
migration?)
But others may think differently.
Hi Boris,
I think allocation of 300KB chunk[order 7 allocation] is more likely to
fail than 70KB[order 5] especially under memory pressure.
If it comes to that, I think we should fix this too.
The no.of rings in most cases remain 4 thanks to xen_blkif_max_queues
module parameter.
If the src host has allocated less than 4[may be vpcu given to this dom0
were less than 4], then we can expect the dst to allocate more than src
side and vice versa.
-Thanks,
Manjunath
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
