| 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
| ||
|
Message-ID: <5374D38A.2070709@citrix.com>
Date: Thu, 15 May 2014 15:47:38 +0100
From: Zoltan Kiss <zoltan.kiss@...rix.com>
To: Wei Liu <wei.liu2@...rix.com>, Jacek Konieczny <jajcus@...cus.net>
CC: <xen-devel@...ts.xen.org>, <netdev@...r.kernel.org>,
Paul Durrant <paul.durrant@...rix.com>
Subject: Re: [PATCH net V2] xen-netback: don't move event pointer in TX credit
timeout callback
On 15/05/14 15:13, Wei Liu wrote:
> On Thu, May 15, 2014 at 03:04:36PM +0200, Jacek Konieczny wrote:
>> On 05/15/14 13:59, Wei Liu wrote:
>>> ... otherwise the frontend will try to send TX event all the time, even
>>> if no progress can be made. The pointer should only be advanced by the
>>> routine that actually processes the ring (that is, xenvif_poll).
>>>
>>> Reported-by: Jacek Konieczny <jajcus@...cus.net>
>>> Signed-off-by: Wei Liu <wei.liu2@...rix.com>
>>> Acked-by: Ian Campbell <ian.campbell@...rix.com>
>>> Cc: Paul Durrant <paul.durrant@...rix.com>
>>> ---
>>> drivers/net/xen-netback/netback.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
>>> index 7666540..8e2cbeb 100644
>>> --- a/drivers/net/xen-netback/netback.c
>>> +++ b/drivers/net/xen-netback/netback.c
>>> @@ -658,7 +658,7 @@ void xenvif_check_rx_xenvif(struct xenvif *vif)
>>> {
>>> int more_to_do;
>>>
>>> - RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do);
>>> + more_to_do = RING_HAS_UNCONSUMED_REQUESTS(&vif->tx);
>>>
>>
>> Unfortunately, this seems not enough to fix the problem I have reported
>> here:
>> http://lists.xenproject.org/archives/html/xen-devel/2014-05/msg01183.html
>>
>> The dom0 network still stalls when using rate limiting on a VIF
>> interface after applying this patch to my 3.14.3 kernel (100% CPU#1
>> usage in the 'soft interrupts').
>>
>
> This is a patch for 3.14.4. I've tested it myself (and looking at the
> right stats!) to confirm it works.
>
> ---8<---
> From a4afed6c44027afff82d6fa7503faef83b01fffe Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@...rix.com>
> Date: Thu, 15 May 2014 15:02:55 +0100
> Subject: [PATCH] xen-netback: call napi_complete if vif is rate limited
>
> Reported-by: Jacek Konieczny <jajcus@...cus.net>
> Signed-off-by: Wei Liu <wei.liu2@...rix.com>
> Cc: Ian Campbell <ian.campbell@...rix.com>
> Cc: Paul Durrant <paul.durrant@...rix.com>
> Cc: David Vrabel <david.vrabel@...rix.com>
> ---
> drivers/net/xen-netback/common.h | 2 +-
> drivers/net/xen-netback/interface.c | 5 +++--
> drivers/net/xen-netback/netback.c | 12 ++++++++----
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 4bf5b33..4c018de 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -219,7 +219,7 @@ void xenvif_check_rx_xenvif(struct xenvif *vif);
> /* Prevent the device from generating any further traffic. */
> void xenvif_carrier_off(struct xenvif *vif);
>
> -int xenvif_tx_action(struct xenvif *vif, int budget);
> +int xenvif_tx_action(struct xenvif *vif, int budget, bool *rate_limited);
>
> int xenvif_kthread(void *data);
> void xenvif_kick_thread(struct xenvif *vif);
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index 2e92d52..03cfbd6 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -61,6 +61,7 @@ static int xenvif_poll(struct napi_struct *napi, int budget)
> {
> struct xenvif *vif = container_of(napi, struct xenvif, napi);
> int work_done;
> + bool rate_limited;
>
> /* This vif is rogue, we pretend we've there is nothing to do
> * for this vif to deschedule it from NAPI. But this interface
> @@ -71,7 +72,7 @@ static int xenvif_poll(struct napi_struct *napi, int budget)
> return 0;
> }
>
> - work_done = xenvif_tx_action(vif, budget);
> + work_done = xenvif_tx_action(vif, budget, &rate_limited);
>
> if (work_done < budget) {
> int more_to_do = 0;
> @@ -96,7 +97,7 @@ static int xenvif_poll(struct napi_struct *napi, int budget)
> local_irq_save(flags);
>
> RING_FINAL_CHECK_FOR_REQUESTS(&vif->tx, more_to_do);
> - if (!more_to_do)
> + if (!more_to_do || rate_limited)
How about calling timer_pending(&vif->credit_timeout) instead?
Also, can this __napi_complete and the callback's napi_schedule race
with each other? When napi_complete is between removing from the list
and clearing the bit, and napi_schedule is just test&set the bit, the
latter won't add the instance to the list again
> __napi_complete(napi);
>
> local_irq_restore(flags);
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 97030c1..fa7d5da 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1125,12 +1125,14 @@ static bool tx_credit_exceeded(struct xenvif *vif, unsigned size)
> return false;
> }
>
> -static unsigned xenvif_tx_build_gops(struct xenvif *vif, int budget)
> +static unsigned xenvif_tx_build_gops(struct xenvif *vif, int budget,
> + bool *rate_limited)
> {
> struct gnttab_copy *gop = vif->tx_copy_ops, *request_gop;
> struct sk_buff *skb;
> int ret;
>
> + *rate_limited = false;
> while ((nr_pending_reqs(vif) + XEN_NETBK_LEGACY_SLOTS_MAX
> < MAX_PENDING_REQS) &&
> (skb_queue_len(&vif->tx_queue) < budget)) {
> @@ -1165,8 +1167,10 @@ static unsigned xenvif_tx_build_gops(struct xenvif *vif, int budget)
>
> /* Credit-based scheduling. */
> if (txreq.size > vif->remaining_credit &&
> - tx_credit_exceeded(vif, txreq.size))
> + tx_credit_exceeded(vif, txreq.size)) {
> + *rate_limited = true;
> break;
> + }
>
> vif->remaining_credit -= txreq.size;
>
> @@ -1382,7 +1386,7 @@ static int xenvif_tx_submit(struct xenvif *vif)
> }
>
> /* Called after netfront has transmitted */
> -int xenvif_tx_action(struct xenvif *vif, int budget)
> +int xenvif_tx_action(struct xenvif *vif, int budget, bool *rate_limited)
> {
> unsigned nr_gops;
> int work_done;
> @@ -1390,7 +1394,7 @@ int xenvif_tx_action(struct xenvif *vif, int budget)
> if (unlikely(!tx_work_todo(vif)))
> return 0;
>
> - nr_gops = xenvif_tx_build_gops(vif, budget);
> + nr_gops = xenvif_tx_build_gops(vif, budget, rate_limited);
>
> if (nr_gops == 0)
> return 0;
>
--
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
Powered by blists - more mailing lists