From: Zoltan Kiss <zoltan.kiss@citrix.com>
To: David Miller <davem@davemloft.net>
Cc: wei.liu2@citrix.com, Ian.Campbell@citrix.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
david.vrabel@citrix.com, xen-devel@lists.xenproject.org
Subject: Re: [PATCH net-next 0/2] xen-netback: Changes around carrier handling
Date: Wed, 6 Aug 2014 20:20:55 +0100 [thread overview]
Message-ID: <53E28017.9040908__1476.95719865641$1407352978$gmane$org@citrix.com> (raw)
In-Reply-To: <20140805.160748.1185917042908283028.davem@davemloft.net>
On 06/08/14 00:07, David Miller wrote:
> From: Zoltan Kiss <zoltan.kiss@citrix.com>
> Date: Mon, 4 Aug 2014 16:20:56 +0100
>
>> This series starts using carrier off as a way to purge packets when the guest is
>> not able (or willing) to receive them. It is a much faster way to get rid of
>> packets waiting for an overwhelmed guest.
>> The first patch changes current netback code where it relies currently on
>> netif_carrier_ok.
>> The second turns off the carrier if the guest times out on a queue, and only
>> turn it on again if that queue (or queues) resurrects.
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>
> Applied, but I have some reservations:
>
> 1) This is starting to bleed what is normally qdisc type policy into the
> driver.
Yeah. The fundamental problem with netback that start_xmit place the
packet into an internal queue, and then the thread does the actual
transmission from that queue, but it doesn't know whether it will
succeed in a finite time period.
There is slot estimation in start_xmit to prevent QDisc pouring more
packets into the internal queue when it seems it won't fit. When that
happens, we stop QDisc and start a timer, and when the timer fires,
before this patch we just ditched the internal queue and started QDisc
again. If the frontend were dead, it lead to a quite long delay until
packets destined to it were freed.
I just noticed a possible problem with start_xmit: if this slot
estimation fails, and the packet is likely to be stalled at least for a
while, it still place the skb into the internal queue and return
NETDEV_TX_OK. Shouldn't we return NETDEV_TX_BUSY and not placing the
packet into the internal queue? It will be requeued later, or dropped by
QDisc. I think it will be more natural. But it can decrease performance
if these "not enough slot" situations are very frequent and short
living, and by the time the RX thread wakes up
My long term idea is to move part of the thread's work into start_xmit,
so it can set up the grant operations and if there isn't enough slot it
can return the skb to QDisc with NETDEV_TX_BUSY for requeueing. Then the
thread can only do the batching of the grant copy operations and
releasing the skbs. And we can ditch a good part of the code ...
>
> 2) There are other drivers that could run into this kind of situation and
> have similar concerns, therefore we should make sure we have a consistent
> approach that such entities use to handle this problem.
>
> Part of the problem is that netif_carrier_off() only partially mimicks
> the situation. It expresses the "transmitter is down so packets
> aren't going onto the wire" part, which keeps the watchdog from
> spitting out log messages ever time it fires. But it doesn't deal
> with packet freeing policy meanwhile, which I guess is the part that
> this patch series is largely trying to address.
>
> Thanks.
>
next prev parent reply other threads:[~2014-08-06 19:21 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-04 15:20 [PATCH net-next 0/2] xen-netback: Changes around carrier handling Zoltan Kiss
2014-08-04 15:20 ` [PATCH net-next 1/2] xen-netback: Using a new state bit instead of carrier Zoltan Kiss
2014-08-05 12:45 ` Wei Liu
2014-08-06 18:25 ` Zoltan Kiss
2014-08-06 18:25 ` Zoltan Kiss
2014-08-05 12:45 ` Wei Liu
2014-08-04 15:20 ` Zoltan Kiss
2014-08-04 15:20 ` [PATCH net-next 2/2] xen-netback: Turn off the carrier if the guest is not able to receive Zoltan Kiss
2014-08-05 12:45 ` Wei Liu
2014-08-06 19:18 ` Zoltan Kiss
2014-08-06 19:18 ` Zoltan Kiss
2014-08-05 12:45 ` Wei Liu
2014-08-04 15:20 ` Zoltan Kiss
2014-08-04 15:34 ` [PATCH net-next 0/2] xen-netback: Changes around carrier handling Zoltan Kiss
2014-08-04 15:34 ` Zoltan Kiss
2014-08-05 23:07 ` David Miller
2014-08-06 0:00 ` Wei Liu
2014-08-06 0:00 ` Wei Liu
2014-08-06 1:50 ` David Miller
2014-08-06 1:50 ` David Miller
2014-08-06 8:54 ` Wei Liu
2014-08-06 8:54 ` Wei Liu
2014-08-06 19:20 ` Zoltan Kiss [this message]
2014-08-06 19:20 ` Zoltan Kiss
2014-08-06 21:01 ` David Miller
2014-08-07 15:51 ` Zoltan Kiss
2014-08-08 5:28 ` David Miller
2014-08-08 5:28 ` David Miller
2014-08-07 15:51 ` Zoltan Kiss
2014-08-06 21:01 ` David Miller
2014-08-07 16:49 ` Zoltan Kiss
2014-08-07 16:49 ` Zoltan Kiss
2014-08-08 5:29 ` David Miller
2014-08-08 5:29 ` David Miller
2014-08-05 23:07 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2014-08-04 15:20 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='53E28017.9040908__1476.95719865641$1407352978$gmane$org@citrix.com' \
--to=zoltan.kiss@citrix.com \
--cc=Ian.Campbell@citrix.com \
--cc=davem@davemloft.net \
--cc=david.vrabel@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.