Xen devel - Jan 2012 - [PATCH] xen-netfront: correct MAX_TX_TARGET calculation.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 drivers/net/xen-netfront.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 38f9c95..01f589d 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -68,7 +68,7 @@ struct netfront_cb {
 
 #define NET_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, PAGE_SIZE)
 #define NET_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, PAGE_SIZE)
-#define TX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
+#define TX_MAX_TARGET min_t(int, NET_TX_RING_SIZE, 256)
 
 struct netfront_stats {
 	u64			rx_packets;
-- 
1.7.8.3

On Thu, Jan 26, 2012 at 05:23:23PM +0000, Wei Liu wrote:

Can you give some more details please? What is the impact of
not having this? Should it be backported to stable?
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  drivers/net/xen-netfront.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 38f9c95..01f589d 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -68,7 +68,7 @@ struct netfront_cb {
>  
>  #define NET_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, PAGE_SIZE)
>  #define NET_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, PAGE_SIZE)
> -#define TX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
> +#define TX_MAX_TARGET min_t(int, NET_TX_RING_SIZE, 256)
>  
>  struct netfront_stats {
>  	u64			rx_packets;
> -- 
> 1.7.8.3

From: Wei Liu <wei.liu2@citrix.com>
Date: Thu, 26 Jan 2012 17:23:23 +0000
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Pretty obvious and straightforward, applied, thanks!

On Thu, 2012-01-26 at 18:19 +0000, Konrad Rzeszutek Wilk
wrote:
> On Thu, Jan 26, 2012 at 05:23:23PM +0000, Wei Liu wrote:
> 
> Can you give some more details please? What is the impact of
> not having this? Should it be backported to stable?
> 
I think it will not cause crash, only the scratch space size is
affected, thus impacting tx batching a bit.

As the tx structure is bigger than rx structure. I think scratch space
size is likely to shrink after correction.


Wei.

On 01/27/12 11:36, Wei Liu wrote:
> On Thu, 2012-01-26 at 18:19 +0000, Konrad Rzeszutek Wilk wrote:
>> On Thu, Jan 26, 2012 at 05:23:23PM +0000, Wei Liu wrote:
>>
>> Can you give some more details please? What is the impact of
>> not having this? Should it be backported to stable?
>>
>
> I think it will not cause crash, only the scratch space size is
> affected, thus impacting tx batching a bit.
>
> As the tx structure is bigger than rx structure. I think scratch space
> size is likely to shrink after correction.
It also seems to affect the netfront_tx_slot_available() function, 
making it stricter (likely). Before the patch, the function may have 
reported available slots when there were none, causing spurious(?) queue 
wakeups in xennet_maybe_wake_tx(), and not stopping the queue in 
xennet_start_xmit() when it should have(?).

It seems there are no further uses of TX_MAX_TARGET, and for bounds 
checking NET_TX_RING_SIZE was used (which was always correct). So I 
guess the typo may have caused some performance degradation.

I can''t either prove or disprove a DoS-like busy loop in the pre-patch
form.

Laszlo

On 02/03/12 13:27, Laszlo Ersek wrote:
> On 01/27/12 11:36, Wei Liu wrote:
>> As the tx structure is bigger than rx structure. I think scratch space
>> size is likely to shrink after correction.
>
> It also seems to affect the netfront_tx_slot_available() function,
> making it stricter (likely). Before the patch, the function may have
> reported available slots when there were none, causing spurious(?) queue
> wakeups in xennet_maybe_wake_tx(), and not stopping the queue in
> xennet_start_xmit() when it should have(?).
(Eyeballing the source makes me think

   NET_TX_RING_SIZE == (4096 - 16 - 48) / (5 * 4) == 201
   NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 4) == 252

but I didn''t try to verify them.)

Laszlo

>>> On 03.02.12 at 13:59, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/03/12 13:27, Laszlo Ersek wrote:
>> On 01/27/12 11:36, Wei Liu wrote:
> 
>>> As the tx structure is bigger than rx structure. I think scratch
space
>>> size is likely to shrink after correction.
>>
>> It also seems to affect the netfront_tx_slot_available() function,
>> making it stricter (likely). Before the patch, the function may have
>> reported available slots when there were none, causing spurious(?)
queue
>> wakeups in xennet_maybe_wake_tx(), and not stopping the queue in
>> xennet_start_xmit() when it should have(?).
> 
> (Eyeballing the source makes me think
> 
>    NET_TX_RING_SIZE == (4096 - 16 - 48) / (5 * 4) == 201
>    NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 4) == 252
NET_TX_RING_SIZE == (4096 - 16 - 48) / (6 * 2) == 336
NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 2) == 504

and with {R,T}X_MAX_TARGET capped to 256 the change really is
benign without multi-page ring support afaict.

Jan
> but I didn''t try to verify them.)
> 
> Laszlo
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel

On 02/03/12 14:26, Jan Beulich wrote:
>>>> On 03.02.12 at 13:59, Laszlo Ersek<lersek@redhat.com> 
wrote:
>> (Eyeballing the source makes me think
>>
>>     NET_TX_RING_SIZE == (4096 - 16 - 48) / (5 * 4) == 201
>>     NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 4) == 252
>
> NET_TX_RING_SIZE == (4096 - 16 - 48) / (6 * 2) == 336
> NET_RX_RING_SIZE == (4096 - 16 - 48) / (4 * 2) == 504
Sorry, I wasn''t sure if gcc would pack them without 
__attribute__((packed)) :) Dumb mistake, admittedly.

Thanks,
Laszlo