From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sasha Levin Subject: Re: [PATCH] virtio-net: fill only rx queues which are being used Date: Tue, 23 Apr 2013 00:49:23 -0400 Message-ID: <517612D3.6090909@oracle.com> References: <1366677336-2278-1-git-send-email-sasha.levin@oracle.com> <87obd5ga0f.fsf@rustcorp.com.au> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: mst@redhat.com, penberg@kernel.org, will.deacon@arm.com, marc.zyngier@arm.com, kvm@vger.kernel.org, asias@redhat.com, jasowang@redhat.com To: Rusty Russell Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:26222 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754760Ab3DWEuO (ORCPT ); Tue, 23 Apr 2013 00:50:14 -0400 In-Reply-To: <87obd5ga0f.fsf@rustcorp.com.au> Sender: kvm-owner@vger.kernel.org List-ID: On 04/23/2013 12:13 AM, Rusty Russell wrote: > Sasha Levin writes: >> Due to MQ support we may allocate a whole bunch of rx queues but >> never use them. With this patch we'll safe the space used by >> the receive buffers until they are actually in use: > > Idea is good, implementation needs a tiny tweak: > >> @@ -912,8 +913,13 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs) >> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d\n", >> queue_pairs); >> return -EINVAL; >> - } else >> + } else { >> + if (queue_pairs > vi->curr_queue_pairs) >> + for (i = 0; i < queue_pairs; i++) >> + if (!try_fill_recv(&vi->rq[i], GFP_KERNEL)) >> + schedule_delayed_work(&vi->refill, 0); >> vi->curr_queue_pairs = queue_pairs; >> + } >> >> return 0; >> } > > You don't want to refill existing queues, so you don't need the "if". > > for (i = vi->curr_queue_pairs; i < queue_pairs; i++) { > if (!try_fill_recv(&vi->rq[i], GFP_KERNEL)) > schedule_delayed_work(&vi->refill, 0); That makes more sense, I'll resend. > We don't free up buffers when we're reducing queues, but I consider that > a corner case. It didn't bother anyone up until now, and the spec doesn't state anything about it - so I preferred to just leave that alone. Unless the ARM folks would find it useful? Thanks, Sasha