[PATCH][RFC] correction trouble of short message in UDP

[PATCH][RFC] correction trouble of short message in UDP

[Tomonari Horikoshi]

[-- Attachment #1: Mail message body --]
[-- Type: text/plain, Size: 2159 bytes --]


Hi all.

I modified the patch to the following trouble.

"When I executed "netperf" by a short message of UDP, 
 PV domain issued Call trace."

This error occurs because earliness grant is filled from 
the queue check of tx_slot when a lot of message is sent. 

I stopped "Add netfornt tx_queue_len" being used, and I was added to check 
gnttab_empty_grant_references() in netfront_tx_slot_available().


I confirmed the not error because of this correction. 
Is there a better correction?


Best regards.

Tomonari Horikoshi,


Tomonari Horikoshi wrote:----------------------
Sent:    Thu, 25 Jan 2007 13:44:30 +0900
Subject: Re: [Xen-devel] [PATCH] Add netfornt tx_queue_len

> Hi Herbert-san
> 
> Thank you for your comment.
> 
> I agreed.
> I examine the another way. 
> 
> It is likely to go well if something is added to the check 
> on netfront_tx_slot_available(). 
> 
> Best regards.
> 
> Tomonari Horikoshi,
> 
> Herbert Xu wrote:----------------------
> Sent:    Wed, 24 Jan 2007 13:29:51 +1100
> Subject: Re: [Xen-devel] [PATCH] Add netfornt tx_queue_len
> 
> > On Wed, Jan 24, 2007 at 01:37:55AM +0000, Tomonari Horikoshi wrote:
> > > 
> > > When I executed "netperf" by a short message of UDP, 
> > > PV domain and PV-on-HVM driver issued Call trace.
> > > 
> > > I think that GrantEntry was filled with a lot of messages processings.
> > > 
> > > This problem is generated in IA64 only.
> > > Probably, I think that I am the following problems. 
> > > 
> > >   In IA64
> > >     NET_TX_RING_SIZE 1024,  NR_GRANT_ENTRIES 2048
> > >   In x86
> > >     NET_TX_RING_SIZE  256,  NR_GRANT_ENTRIES 2048
> > > 
> > > I corrected to check "number of unprocessing queue > tx_queue_len" before Grant was filled.
> > > 
> > > However, my correction influences x86. 
> > > Please teach to me in that when there is a better improvement. 
> > 
> > Sorry, but this patch looks bogus.  The tx queue is maintained by
> > Linux and has nothing to do with the driver.  So limiting its length
> > based on internal state of the driver can't be right.
> > 
> > We need to find out what's really going wrong with the grant table
> > entries here.
> > 
> > Cheers,



[-- Attachment #2: netfront_chk_grant.patch --]
[-- Type: application/octet-stream, Size: 825 bytes --]

# HG changeset patch
# User t.horikoshi@jp.fujisu.com
# Date 1169712349 -32400
# Node ID fef672b56e77ed6ce66fc6b97b89bc4e8c677b6a
# Parent  b4df7de0cbf7530c3248956243bc9c520db2d609
netfront add check grant reference

Signed-off-by: Tomonari Horikoshi <t.horikoshi@jp.fujitsu.com>

diff -r b4df7de0cbf7 -r fef672b56e77 linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
--- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Wed Jan 24 12:28:05 2007 -0700
+++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Thu Jan 25 17:05:49 2007 +0900
@@ -613,6 +613,9 @@ static int network_open(struct net_devic
 
 static inline int netfront_tx_slot_available(struct netfront_info *np)
 {
+	if( gnttab_empty_grant_references(&np->gref_tx_head) )
+		return 0;
+
 	return RING_FREE_REQUESTS(&np->tx) >= MAX_SKB_FRAGS + 2;
 }
 

[-- Attachment #3: Type: text/plain, Size: 152 bytes --]

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel

Re: [PATCH][RFC] correction trouble of short message in UDP

[Keir Fraser]

On 25/1/07 11:04, "Tomonari Horikoshi" <t.horikoshi@jp.fujitsu.com> wrote:

> I confirmed the not error because of this correction.
> Is there a better correction?

It's along the right lines so I check it in as is. However, you probably
need to check that you have enough grant entries for worst-case packet
fragmentation (a packet may require more than one grant). Or start_xmit()
needs to be able to buffer one packet until it has enough grants to proceed
with its transmission.

 -- Keir

Re: [PATCH][RFC] correction trouble of short message in UDP

[Herbert Xu]

Keir Fraser <keir@xensource.com> wrote:
> On 25/1/07 11:04, "Tomonari Horikoshi" <t.horikoshi@jp.fujitsu.com> wrote:
> 
>> I confirmed the not error because of this correction.
>> Is there a better correction?
> 
> It's along the right lines so I check it in as is. However, you probably
> need to check that you have enough grant entries for worst-case packet
> fragmentation (a packet may require more than one grant). Or start_xmit()
> needs to be able to buffer one packet until it has enough grants to proceed
> with its transmission.

How about making the total number of grant table entries equal the
number of TX slots? That way we won't have to check anything at all.

It also doesn't make sense to have more TX slots than grant table entries.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH][RFC] correction trouble of short message in UDP

[Keir Fraser]

On 25/1/07 13:40, "Herbert Xu" <herbert@gondor.apana.org.au> wrote:

> How about making the total number of grant table entries equal the
> number of TX slots? That way we won't have to check anything at all.
> 
> It also doesn't make sense to have more TX slots than grant table entries.

Yes, this is a good idea!

So we should replace current netfront_tx_slot_available implementation with:
 (tx->req_prod_pvt - tx->rsp_cons) < (TX_MAX_TARGET - MAX_SKB_FRAGS - 2)

??

 -- Keir

Re: [PATCH][RFC] correction trouble of short message in UDP

[Keir Fraser]

On 25/1/07 14:03, "Keir Fraser" <keir@xensource.com> wrote:

>> How about making the total number of grant table entries equal the
>> number of TX slots? That way we won't have to check anything at all.
>> 
>> It also doesn't make sense to have more TX slots than grant table entries.
> 
> Yes, this is a good idea!
> 
> So we should replace current netfront_tx_slot_available implementation with:
>  (tx->req_prod_pvt - tx->rsp_cons) < (TX_MAX_TARGET - MAX_SKB_FRAGS - 2)
> 
> ??

Sorry, what I'm suggesting actually isn't the same as your proposal. :-)

I think mine would work though, and 256 grants should be plenty as a
maximum.

 -- Keir

Re: [PATCH][RFC] correction trouble of short message in UDP

[Herbert Xu]

On Thu, Jan 25, 2007 at 02:03:19PM +0000, Keir Fraser wrote:
> 
> So we should replace current netfront_tx_slot_available implementation with:
>  (tx->req_prod_pvt - tx->rsp_cons) < (TX_MAX_TARGET - MAX_SKB_FRAGS - 2)

Yeah that's the idea.  I was thinking of something like this:

diff -r bea505a69722 linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
--- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Jan 23 15:58:05 2007 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Fri Jan 26 01:14:19 2007 +1100
@@ -138,8 +138,10 @@ static inline int netif_needs_gso(struct
 
 #define GRANT_INVALID_REF	0
 
-#define NET_TX_RING_SIZE __RING_SIZE((struct netif_tx_sring *)0, PAGE_SIZE)
-#define NET_RX_RING_SIZE __RING_SIZE((struct netif_rx_sring *)0, PAGE_SIZE)
+#define NET_TX_RING_SIZE \
+	(__RING_SIZE((struct netif_tx_sring *)0, PAGE_SIZE) & 255)
+#define NET_RX_RING_SIZE \
+	(__RING_SIZE((struct netif_rx_sring *)0, PAGE_SIZE) & 255)
 
 struct netfront_info {
 	struct list_head list;
@@ -159,7 +161,7 @@ struct netfront_info {
 	/* Receive-ring batched refills. */
 #define RX_MIN_TARGET 8
 #define RX_DFL_MIN_TARGET 64
-#define RX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
+#define RX_MAX_TARGET NET_RX_RING_SIZE
 	unsigned rx_min_target, rx_max_target, rx_target;
 	struct sk_buff_head rx_batch;
 
@@ -172,7 +174,7 @@ struct netfront_info {
 	struct sk_buff *tx_skbs[NET_TX_RING_SIZE+1];
 	struct sk_buff *rx_skbs[NET_RX_RING_SIZE];
 
-#define TX_MAX_TARGET min_t(int, NET_RX_RING_SIZE, 256)
+#define TX_MAX_TARGET NET_TX_RING_SIZE
 	grant_ref_t gref_tx_head;
 	grant_ref_t grant_tx_ref[NET_TX_RING_SIZE + 1];
 	grant_ref_t gref_rx_head;

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH][RFC] correction trouble of short message in UDP

[Herbert Xu]

On Fri, Jan 26, 2007 at 01:17:19AM +1100, Herbert Xu wrote:
> 
> diff -r bea505a69722 linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c
> --- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Tue Jan 23 15:58:05 2007 +0000
> +++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Fri Jan 26 01:14:19 2007 +1100
> @@ -138,8 +138,10 @@ static inline int netif_needs_gso(struct
>  
>  #define GRANT_INVALID_REF	0
>  
> -#define NET_TX_RING_SIZE __RING_SIZE((struct netif_tx_sring *)0, PAGE_SIZE)
> -#define NET_RX_RING_SIZE __RING_SIZE((struct netif_rx_sring *)0, PAGE_SIZE)
> +#define NET_TX_RING_SIZE \
> +	(__RING_SIZE((struct netif_tx_sring *)0, PAGE_SIZE) & 255)
> +#define NET_RX_RING_SIZE \
> +	(__RING_SIZE((struct netif_rx_sring *)0, PAGE_SIZE) & 255)

Actually this can't work because the size is independently derived in
two different places...

So your idea definitely sounds better.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt