All of lore.kernel.org
 help / color / mirror / Atom feed
From: Zoltan Kiss <zoltan.kiss@citrix.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: xen-devel@lists.xenproject.org, jonathan.davies@citrix.com,
	ian.campbell@citrix.com, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH net-next v2 8/9] xen-netback: Timeout packets in RX path
Date: Mon, 16 Dec 2013 17:16:17 +0000	[thread overview]
Message-ID: <52AF3561.9020904__17276.8641532913$1387214276$gmane$org@citrix.com> (raw)
In-Reply-To: <20131213154406.GO21900@zion.uk.xensource.com>

On 13/12/13 15:44, Wei Liu wrote:
> On Thu, Dec 12, 2013 at 11:48:16PM +0000, Zoltan Kiss wrote:
>> A malicious or buggy guest can leave its queue filled indefinitely, in which
>> case qdisc start to queue packets for that VIF. If those packets came from an
>> another guest, it can block its slots and prevent shutdown. To avoid that, we
>> make sure the queue is drained in every 10 seconds
>>
>
> Oh I see where the 10 second constraint in previous patch comes from.
>
> Could you define a macro for this constant then use it everywhere.
Well, they are not entirely the same thing, but worth making them the 
same. How about using "unmap_timeout > (rx_drain_timeout_msecs/1000)" in 
xenvif_free()? Then netback won't complain about a stucked page if an 
another guest is permitted to hold on to it.

>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>> ---
> [...]
>> +static void xenvif_wake_queue(unsigned long data)
>> +{
>> +	struct xenvif *vif = (struct xenvif *)data;
>> +
>> +	netdev_err(vif->dev, "timer fires\n");
>
> What timer? This error message needs to be more specific.
I forgot to remove this, I used it for debugging only. The other message 
2 line below is more important

>
>> +	if (netif_queue_stopped(vif->dev)) {
>> +		netdev_err(vif->dev, "draining TX queue\n");
>> +		netif_wake_queue(vif->dev);
>> +	}
>> +}
>> +
>>   static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>   {
>>   	struct xenvif *vif = netdev_priv(dev);
>> @@ -141,8 +152,13 @@ static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>   	 * then turn off the queue to give the ring a chance to
>>   	 * drain.
>>   	 */
>> -	if (!xenvif_rx_ring_slots_available(vif, min_slots_needed))
>> +	if (!xenvif_rx_ring_slots_available(vif, min_slots_needed)) {
>> +		vif->wake_queue.function = xenvif_wake_queue;
>> +		vif->wake_queue.data = (unsigned long)vif;
>>   		xenvif_stop_queue(vif);
>> +		mod_timer(&vif->wake_queue,
>> +			jiffies + rx_drain_timeout_jiffies);
>> +	}
>>
>
> Do you need to use jiffies_64 instead of jiffies?
Well, we don't use time_after_eq here, just set the timer. AFAIK that 
should be OK.

> This timer is only armed when ring is full. So what happens when the
> ring is not full and some other parts of the system holds on to the
> packets forever? Can this happen?
This timer is not to protect the receiving guest, but to protect the 
sender. If the ring is not full, then netback will put the packet there 
and release the skb back.
This patch is to replace delayed copy from classic kernel times. There 
we handled this problem on the sender side: after a timer expired we 
made a local copy of the packet and released back the pages. It had 
stronger guarantees that a guest will always get back its pages, but it 
also caused more unnecessary copies when the system is already loaded 
and we should really thrash the packet. Unfortunately we can't do that 
as the sender is no longer in control.
Instead I choose this more lightweight solution, because practice said 
an another guest's queue is the only place where the packet can get 
stucked, especially if that guest is malicious, buggy, or too slow.
Other parts (e.g. a driver) can also hold on the packet if they are 
buggy, but then we should fix that bug rather than feed it with more 
guest pages.

Zoli

  parent reply	other threads:[~2013-12-16 17:16 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-12 23:48 [PATCH net-next v2 0/9] xen-netback: TX grant mapping with SKBTX_DEV_ZEROCOPY instead of copy Zoltan Kiss
2013-12-12 23:48 ` [PATCH net-next v2 1/9] xen-netback: Introduce TX grant map definitions Zoltan Kiss
2013-12-13 15:31   ` Wei Liu
2013-12-13 15:31   ` Wei Liu
2013-12-13 18:22     ` Zoltan Kiss
2013-12-13 18:22     ` Zoltan Kiss
2013-12-13 19:14       ` Wei Liu
2013-12-13 19:14       ` Wei Liu
2013-12-16 15:21         ` Zoltan Kiss
2013-12-16 15:21         ` Zoltan Kiss
2013-12-16 17:50           ` Wei Liu
2014-01-07 14:50             ` Zoltan Kiss
2014-01-07 14:50             ` Zoltan Kiss
2013-12-16 17:50           ` Wei Liu
2013-12-12 23:48 ` Zoltan Kiss
2013-12-12 23:48 ` [PATCH net-next v2 2/9] xen-netback: Change TX path from grant copy to mapping Zoltan Kiss
2013-12-12 23:48   ` Zoltan Kiss
2013-12-13 15:36   ` Wei Liu
2013-12-13 15:36   ` Wei Liu
2013-12-16 15:38     ` Zoltan Kiss
2013-12-16 18:21       ` Wei Liu
2013-12-16 18:57         ` Zoltan Kiss
2013-12-16 19:06           ` Wei Liu
2013-12-16 19:06           ` Wei Liu
2013-12-16 18:57         ` Zoltan Kiss
2013-12-16 18:21       ` Wei Liu
2013-12-16 15:38     ` Zoltan Kiss
2013-12-17 21:49   ` [Xen-devel] " Konrad Rzeszutek Wilk
2013-12-30 17:58     ` Zoltan Kiss
2013-12-30 17:58     ` [Xen-devel] " Zoltan Kiss
2013-12-17 21:49   ` Konrad Rzeszutek Wilk
2013-12-12 23:48 ` [PATCH net-next v2 3/9] xen-netback: Remove old TX grant copy definitons and fix indentations Zoltan Kiss
2013-12-12 23:48   ` Zoltan Kiss
2013-12-12 23:48 ` [PATCH net-next v2 4/9] xen-netback: Change RX path for mapped SKB fragments Zoltan Kiss
2013-12-12 23:48 ` Zoltan Kiss
2013-12-12 23:48 ` [PATCH net-next v2 5/9] xen-netback: Add stat counters for zerocopy Zoltan Kiss
2013-12-12 23:48 ` Zoltan Kiss
2013-12-12 23:48 ` [PATCH net-next v2 6/9] xen-netback: Handle guests with too many frags Zoltan Kiss
2013-12-12 23:48 ` Zoltan Kiss
2013-12-13 15:43   ` Wei Liu
2013-12-13 15:43   ` Wei Liu
2013-12-16 16:10     ` Zoltan Kiss
2013-12-16 16:10     ` Zoltan Kiss
2013-12-16 18:09       ` Wei Liu
2014-01-07 15:23         ` Zoltan Kiss
2014-01-07 15:23         ` Zoltan Kiss
2013-12-16 18:09       ` Wei Liu
2013-12-12 23:48 ` [PATCH net-next v2 7/9] xen-netback: Add stat counters for frag_list skbs Zoltan Kiss
2013-12-12 23:48 ` Zoltan Kiss
2013-12-12 23:48 ` [PATCH net-next v2 8/9] xen-netback: Timeout packets in RX path Zoltan Kiss
2013-12-13 15:44   ` Wei Liu
2013-12-16 17:16     ` Zoltan Kiss
2013-12-16 19:03       ` Wei Liu
2013-12-16 19:03       ` Wei Liu
2013-12-16 17:16     ` Zoltan Kiss [this message]
2013-12-13 15:44   ` Wei Liu
2013-12-12 23:48 ` Zoltan Kiss
2013-12-12 23:48 ` [PATCH net-next v2 9/9] xen-netback: Aggregate TX unmap operations Zoltan Kiss
2013-12-13 15:44   ` Wei Liu
2013-12-13 15:44   ` Wei Liu
2013-12-16 16:30     ` Zoltan Kiss
2013-12-16 16:30     ` Zoltan Kiss
2013-12-12 23:48 ` Zoltan Kiss
2013-12-16  6:32 ` [PATCH net-next v2 0/9] xen-netback: TX grant mapping with SKBTX_DEV_ZEROCOPY instead of copy annie li
2013-12-16  6:32 ` [Xen-devel] " annie li
2013-12-16 16:13   ` Zoltan Kiss
2013-12-16 16:13   ` Zoltan Kiss

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='52AF3561.9020904__17276.8641532913$1387214276$gmane$org@citrix.com' \
    --to=zoltan.kiss@citrix.com \
    --cc=ian.campbell@citrix.com \
    --cc=jonathan.davies@citrix.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=wei.liu2@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.