netdev - Re: [PATCH net-next v4 8/9] xen-netback: Timeout packets in RX path

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]

Date:	Fri, 17 Jan 2014 19:27:35 +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 v4 8/9] xen-netback: Timeout packets in RX path

On 16/01/14 00:03, Wei Liu wrote:
> On Tue, Jan 14, 2014 at 08:39:54PM +0000, Zoltan Kiss wrote:
> [...]
>> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
>> index 109c29f..d1cd8ce 100644
>> --- a/drivers/net/xen-netback/common.h
>> +++ b/drivers/net/xen-netback/common.h
>> @@ -129,6 +129,9 @@ struct xenvif {
>>   	struct xen_netif_rx_back_ring rx;
>>   	struct sk_buff_head rx_queue;
>>   	RING_IDX rx_last_skb_slots;
>
> Hmm... You seemed to mix your other patch with this series. :-)
Yep, this series doesn't work without that patch (actually that is a bug 
in netback even without my series), so at the moment it is based on it.

>
>> +	bool rx_queue_purge;
>> +
>> +	struct timer_list wake_queue;
>>
>>   	/* This array is allocated seperately as it is large */
>>   	struct gnttab_copy *grant_copy_op;
>> @@ -225,4 +228,7 @@ void xenvif_idx_unmap(struct xenvif *vif, u16 pending_idx);
>>
>>   extern bool separate_tx_rx_irq;
>>
> [...]
>> @@ -559,7 +579,7 @@ void xenvif_free(struct xenvif *vif)
>>   		if (vif->grant_tx_handle[i] != NETBACK_INVALID_HANDLE) {
>>   			unmap_timeout++;
>>   			schedule_timeout(msecs_to_jiffies(1000));
>> -			if (unmap_timeout > 9 &&
>> +			if (unmap_timeout > ((rx_drain_timeout_msecs/1000) * DIV_ROUND_UP(XENVIF_QUEUE_LENGTH, (XEN_NETIF_RX_RING_SIZE / MAX_SKB_FRAGS))) &&
>
> This line is really too long. And what's the rationale behind this long
> expression?
It calculates how many times you should ditch the internal queue of an 
another (maybe stucked) vif before Qdisc empties it's actual content. 
After that there shouldn't be any mapped handle left, so we should start 
printing these messages. Actually it should use vif->dev->tx_queue_len, 
and yes, it is probably better to move it to the beginning of the 
function into a new variable, and use that here.

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