netdev - Re: [PATCH net-next v2 9/9] xen-netback: Aggregate TX unmap operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52AF2A8A.50404@citrix.com>
Date:	Mon, 16 Dec 2013 16:30:02 +0000
From:	Zoltan Kiss <zoltan.kiss@...rix.com>
To:	Wei Liu <wei.liu2@...rix.com>
CC:	<ian.campbell@...rix.com>, <xen-devel@...ts.xenproject.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<jonathan.davies@...rix.com>
Subject: Re: [PATCH net-next v2 9/9] xen-netback: Aggregate TX unmap operations

On 13/12/13 15:44, Wei Liu wrote:
> On Thu, Dec 12, 2013 at 11:48:17PM +0000, Zoltan Kiss wrote:
>> Unmapping causes TLB flushing, therefore we should make it in the largest
>> possible batches. However we shouldn't starve the guest for too long. So if
>> the guest has space for at least two big packets and we don't have at least a
>> quarter ring to unmap, delay it for at most 1 milisec.
>>
>
> Is this solution temporary or permanent? If it is permanent would it
> make sense to make these parameter tunable?

Well, I'm not entirely sure yet this is the best way to do this, so in 
this sense it's temporary. But generally we should do some sort of 
batching, as TLB flush cannot be avoided every time. If we settle on 
something we should make the tunable parameters tunable.
The problem is that it is a thin red line we should find here. My first 
approach was that I left the tx_dealloc_work_todo as it was, and after 
the thread woke up but before anything were done I made it sleep for 50 
ns and measured  how fast the guest is running out of free slots:

if (kthread_should_stop())
	break;

+i=0;
+do {
+	++i;
+	prev_free_slots = nr_free_slots(&vif->tx);
+	__set_current_state(TASK_UNINTERRUPTIBLE);
+	rc = schedule_hrtimeout_range(&tx_dealloc_delay_ktime, 10, 
HRTIMER_MODE_REL);
+	if (rc) trace_printk("%s sleep were interrupted! %d\n",vif->dev->name, 
rc);
+	curr_free_slots = nr_free_slots(&vif->tx);
+} while ( (curr_free_slots < 4 * (prev_free_slots - curr_free_slots)) 
&& i < 11);
+
xenvif_tx_dealloc_action(vif);

And worst case after 500 ns I let the thread to do the unmap anyway. But 
I was a bit worried about this approach, so I choose a bit more 
conservative one for this patch.

There are also ideas to use some other instrument for unmapping instead 
of the current separate thread approach. Putting it into the NAPI 
instance was the original idea, which caused problems. Placing it into 
the another thread where RX work happens also doesn't sound too good, 
these things can and should happen in parallel.
Other ideas were work queues and tasklets, I'll spend some more time to 
check if they are feasible.

Regards,

Zoli
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html