On Mon, Jul 20, 2015 at 05:42:11PM +0300, Ossi Herrala wrote:
On Sat, Jun 06, 2015 at 07:36:48PM +0000, Ossi Herrala wrote: Sorry to miss this mail, it got buried somehow and I haven't got to it until now since nobody pinged it. Sorry for the long wait then.No worries and thank you for taking time to review my patch. See new patch attached as well as comments on the memory usage below. The patch is tested on latest master (e46791e003444ce825feaf5bb2a16f778ee951e5).The only thing I would mention wrt to how it works after this patch is that it will consume some memory that's not needed, precisely (sizeof(struct iovec) + sizeof(void *)) * unreadMBs. It might be worth it to do: memmove(st->incomingVec, st->incomingVec + st->readVec, st->writeVec - st->readVec); VIR_SHRINK_N(st->incomingVec, st->readVec); st->writeVec -= st->readVec; st->readVec = 0; Apart from that it's definitely *way* better approach. What do you think of such modification? This would go either instead 'st->readVec++', but rather at the end of this function, so it's not done after each MB read.I totally agree and implemented freeing of the memory in the new patch: if (st->readVec >= 16) { memmove(st->incomingVec, st->incomingVec + st->readVec, sizeof(*st->incomingVec)*(st->writeVec - st->readVec)); VIR_SHRINK_N(st->incomingVec, st->writeVec, st->readVec); VIR_DEBUG("shrink removed %zu, left %zu", st->readVec, st->writeVec); st->readVec = 0; } now it only frees memory in chunks of 16 to avoid doing memmove() all the time. The iovec is 16 bytes (in 64 bit Linux) so this frees 256 bytes at a time and in my testing usually everything or almost everything that is allocated. Thanks again for taking time to review and commit, and I hope the new patch below is formatted ok.
I managed to apply it as needed. I like how it works now and the feeing is fine, too. I tested it and it seems it's working perfectly. Without this patch I've got stuck on 1MiB/s locally after 11GiB in circa 1 minute, with this patch I managed to download 20GiB in less than a minute. ACK && will push after release since we're way after rc2 already. Again, sorry for the delays and thanks for the contribution! Have a nice day, Martin
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list