[PATCH 2/3] r8169: code clean-up

[PATCH 2/3] r8169: code clean-up

[Jon Mason]

This patch removes netif_poll_disable if NAPI is not enabled (otherwise 
adapter will hang while changing MTUs).  This patch also fixes a possible skb
alignment overrun, and fixes the rx skb allocation error logging. It also 
removes an unnecessary define, adds a link down notification, and cleans up 
some comments. 

It is all pretty trivial, but let me know if this is too much for one patch.

Tested on amd64 (and very lightly tested on x86).

Applies cleanly to linux-2.6.11-rc2-mm2 version of the driver.

Signed-off-by: Jon Mason <jdmason@us.ibm.com>

--- drivers/net/r8169.c.orig 2005-02-16 17:16:19.000000000 -0600
+++ drivers/net/r8169.c 2005-02-16 17:17:40.000000000 -0600
@@ -84,10 +84,12 @@ VERSION 1.6LK <2004/04/14>
 #define rtl8169_rx_skb   netif_receive_skb
 #define rtl8169_rx_hwaccel_skb  vlan_hwaccel_rx
 #define rtl8169_rx_quota(count, quota) min(count, quota)
+#define netif_poll_disable(dev)  netif_poll_disable(dev)
 #else
 #define rtl8169_rx_skb   netif_rx
 #define rtl8169_rx_hwaccel_skb  vlan_hwaccel_receive_skb
 #define rtl8169_rx_quota(count, quota) count
+#define netif_poll_disable(dev)
 #endif
 
 /* media options */
@@ -102,11 +104,9 @@ static int max_interrupt_work = 20;
    The RTL chips use a 64 element hash table based on the Ethernet CRC.  */
 static int multicast_filter_limit = 32;
 
-/* MAC address length*/
+/* MAC address length */
 #define MAC_ADDR_LEN 6
 
-#define TX_FIFO_THRESH 256 /* In bytes */
-
 #define RX_FIFO_THRESH 7 /* 7 means NO threshold, Rx buffer level before first PCI xfer.  */
 #define RX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */
 #define TX_DMA_BURST 6 /* Maximum PCI burst, '6' is 1024 */
@@ -521,8 +521,10 @@ static void rtl8169_check_link_status(st
  if (tp->link_ok(ioaddr)) {
   netif_carrier_on(dev);
   printk(KERN_INFO PFX "%s: link up\n", dev->name);
- } else
+ } else {
   netif_carrier_off(dev);
+  printk(KERN_INFO PFX "%s: link down\n", dev->name);
+ } 
  spin_unlock_irqrestore(&tp->lock, flags);
 }
 
@@ -1549,10 +1551,10 @@ rtl8169_hw_start(struct net_device *dev)
  RTL_W8(ChipCmd, CmdTxEnb | CmdRxEnb);
  RTL_W8(EarlyTxThres, EarlyTxThld);
 
- // For gigabit rtl8169, MTU + header + CRC + VLAN
+ /* For gigabit rtl8169, MTU + header + CRC + VLAN */
  RTL_W16(RxMaxSize, tp->rx_buf_sz);
 
- // Set Rx Config register
+ /* Set Rx Config register */
  i = rtl8169_rx_config |
   (RTL_R32(RxConfig) & rtl_chip_info[tp->chipset].RxConfigMask);
  RTL_W32(RxConfig, i);
@@ -1659,11 +1661,11 @@ static int rtl8169_alloc_rx_skb(struct p
  dma_addr_t mapping;
  int ret = 0;
 
- skb = dev_alloc_skb(rx_buf_sz);
+ skb = dev_alloc_skb(rx_buf_sz + NET_IP_ALIGN);
  if (!skb)
   goto err_out;
 
- skb_reserve(skb, 2);
+ skb_reserve(skb, NET_IP_ALIGN);
  *sk_buff = skb;
 
  mapping = pci_map_single(pdev, skb->tail, rx_buf_sz,
@@ -2189,7 +2191,7 @@ rtl8169_rx_interrupt(struct net_device *
  tp->cur_rx = cur_rx;
 
  delta = rtl8169_rx_fill(tp, dev, tp->dirty_rx, tp->cur_rx);
- if (delta < 0) {
+ if (delta < 1 && count) {
   printk(KERN_INFO "%s: no Rx buffer allocated\n", dev->name);
   delta = 0;
  }

Re: [PATCH 2/3] r8169: code clean-up

[Francois Romieu]

Jon Mason <jdmason@us.ibm.com> :
> This patch removes netif_poll_disable if NAPI is not enabled (otherwise 
> adapter will hang while changing MTUs).

I am currently running a non-napi r8169 on x86/sparc64 based on 2.6.11-rc4
+ patches in Jeff's netdev and it apparently does not mind change of mtu.

How am I supposed to make it hang ?

> This patch also fixes a possible skb alignment overrun,

Ok. Added some bits, see below.

> and fixes the rx skb allocation error logging. It also 

Ok. I'd rather see it as shown below though (cuts some code and more
ppc-friendly unsigned ints).

> removes an unnecessary define,

Ok.

> adds a link down notification, and cleans up 

Ok. I'll netif_msg it before someone else complains that the driver
is too verbose.

----------------------8<-----------------------------------------

Fix rx skb allocation error logging

Signed arithmetic is not required as rtl8169_rx_fill() return belongs
to the [0; NUM_RX_DESC] interval.

Signed-off-by: Jon Mason <jdmason@us.ibm.com>
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

diff -puN drivers/net/r8169.c~r8169-400 drivers/net/r8169.c
--- a/drivers/net/r8169.c~r8169-400	2005-02-17 21:50:09.820173216 +0100
+++ b/drivers/net/r8169.c	2005-02-17 22:00:00.210450784 +0100
@@ -2156,8 +2156,8 @@ static int
 rtl8169_rx_interrupt(struct net_device *dev, struct rtl8169_private *tp,
 		     void __iomem *ioaddr)
 {
-	unsigned int cur_rx, rx_left, count;
-	int delta;
+	unsigned int cur_rx, rx_left;
+	unsigned int delta, count;
 
 	assert(dev != NULL);
 	assert(tp != NULL);
@@ -2225,10 +2225,8 @@ rtl8169_rx_interrupt(struct net_device *
 	tp->cur_rx = cur_rx;
 
 	delta = rtl8169_rx_fill(tp, dev, tp->dirty_rx, tp->cur_rx);
-	if (delta < 0) {
+	if (!delta && count)
 		printk(KERN_INFO "%s: no Rx buffer allocated\n", dev->name);
-		delta = 0;
-	}
 	tp->dirty_rx += delta;
 
 	/*

_

Nail an overrun in skb alignment and remove the relevant magic variable.

Signed-off-by: Jon Mason <jdmason@us.ibm.com>
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

diff -puN drivers/net/r8169.c~r8169-410 drivers/net/r8169.c
--- a/drivers/net/r8169.c~r8169-410	2005-02-17 22:13:04.209897364 +0100
+++ b/drivers/net/r8169.c	2005-02-17 22:17:25.747969121 +0100
@@ -1697,11 +1697,11 @@ static int rtl8169_alloc_rx_skb(struct p
 	dma_addr_t mapping;
 	int ret = 0;
 
-	skb = dev_alloc_skb(rx_buf_sz);
+	skb = dev_alloc_skb(rx_buf_sz + NET_IP_ALIGN);
 	if (!skb)
 		goto err_out;
 
-	skb_reserve(skb, 2);
+	skb_reserve(skb, NET_IP_ALIGN);
 	*sk_buff = skb;
 
 	mapping = pci_map_single(pdev, skb->tail, rx_buf_sz,
@@ -2140,9 +2140,9 @@ static inline int rtl8169_try_rx_copy(st
 	if (pkt_size < rx_copybreak) {
 		struct sk_buff *skb;
 
-		skb = dev_alloc_skb(pkt_size + 2);
+		skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
 		if (skb) {
-			skb_reserve(skb, 2);
+			skb_reserve(skb, NET_IP_ALIGN);
 			eth_copy_and_sum(skb, sk_buff[0]->tail, pkt_size, 0);
 			*sk_buff = skb;
 			rtl8169_return_to_asic(desc, rx_buf_sz);

_


--
Ueimor

Re: [PATCH 2/3] r8169: code clean-up

[Jon Mason]

On Thursday 17 February 2005 05:28 pm, Francois Romieu wrote:
> Jon Mason <jdmason@us.ibm.com> :
> > This patch removes netif_poll_disable if NAPI is not enabled (otherwise
> > adapter will hang while changing MTUs).
>
> I am currently running a non-napi r8169 on x86/sparc64 based on 2.6.11-rc4
> + patches in Jeff's netdev and it apparently does not mind change of mtu.
>
> How am I supposed to make it hang ?

I can't seem to make it hang anymore.  I guess I was wrong.  Please remove 
this part of the patch.

> > This patch also fixes a possible skb alignment overrun,
>
> Ok. Added some bits, see below.

Sorry for the oversight, but I had the 2nd part in the jumbo frames patch.

> > and fixes the rx skb allocation error logging. It also
>
> Ok. I'd rather see it as shown below though (cuts some code and more
> ppc-friendly unsigned ints).

Actually, I think it should be something like:
if (delta != count)

> > removes an unnecessary define,
>
> Ok.
>
> > adds a link down notification, and cleans up
>
> Ok. I'll netif_msg it before someone else complains that the driver
> is too verbose.

ya, I was thinking about modifying most of the printks to dprintks (and 
possibly moving to the e1000 dprintk model),  but I like the link down 
notification.

> ----------------------8<-----------------------------------------
>
> Fix rx skb allocation error logging
>
> Signed arithmetic is not required as rtl8169_rx_fill() return belongs
> to the [0; NUM_RX_DESC] interval.
>
> Signed-off-by: Jon Mason <jdmason@us.ibm.com>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
>
> diff -puN drivers/net/r8169.c~r8169-400 drivers/net/r8169.c
> --- a/drivers/net/r8169.c~r8169-400 2005-02-17 21:50:09.820173216 +0100
> +++ b/drivers/net/r8169.c 2005-02-17 22:00:00.210450784 +0100
> @@ -2156,8 +2156,8 @@ static int
>  rtl8169_rx_interrupt(struct net_device *dev, struct rtl8169_private *tp,
>         void __iomem *ioaddr)
>  {
> - unsigned int cur_rx, rx_left, count;
> - int delta;
> + unsigned int cur_rx, rx_left;
> + unsigned int delta, count;
>
>   assert(dev != NULL);
>   assert(tp != NULL);
> @@ -2225,10 +2225,8 @@ rtl8169_rx_interrupt(struct net_device *
>   tp->cur_rx = cur_rx;
>
>   delta = rtl8169_rx_fill(tp, dev, tp->dirty_rx, tp->cur_rx);
> - if (delta < 0) {
> + if (!delta && count)
>    printk(KERN_INFO "%s: no Rx buffer allocated\n", dev->name);
> -  delta = 0;
> - }
>   tp->dirty_rx += delta;
>
>   /*
>
> _
>
> Nail an overrun in skb alignment and remove the relevant magic variable.
>
> Signed-off-by: Jon Mason <jdmason@us.ibm.com>
> Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
>
> diff -puN drivers/net/r8169.c~r8169-410 drivers/net/r8169.c
> --- a/drivers/net/r8169.c~r8169-410 2005-02-17 22:13:04.209897364 +0100
> +++ b/drivers/net/r8169.c 2005-02-17 22:17:25.747969121 +0100
> @@ -1697,11 +1697,11 @@ static int rtl8169_alloc_rx_skb(struct p
>   dma_addr_t mapping;
>   int ret = 0;
>
> - skb = dev_alloc_skb(rx_buf_sz);
> + skb = dev_alloc_skb(rx_buf_sz + NET_IP_ALIGN);
>   if (!skb)
>    goto err_out;
>
> - skb_reserve(skb, 2);
> + skb_reserve(skb, NET_IP_ALIGN);
>   *sk_buff = skb;
>
>   mapping = pci_map_single(pdev, skb->tail, rx_buf_sz,
> @@ -2140,9 +2140,9 @@ static inline int rtl8169_try_rx_copy(st
>   if (pkt_size < rx_copybreak) {
>    struct sk_buff *skb;
>
> -  skb = dev_alloc_skb(pkt_size + 2);
> +  skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
>    if (skb) {
> -   skb_reserve(skb, 2);
> +   skb_reserve(skb, NET_IP_ALIGN);
>     eth_copy_and_sum(skb, sk_buff[0]->tail, pkt_size, 0);
>     *sk_buff = skb;
>     rtl8169_return_to_asic(desc, rx_buf_sz);
>
> _
>
>
> --
> Ueimor

Re: [PATCH 2/3] r8169: code clean-up

[Richard Dawe]

Hello.

Francois Romieu wrote:
> Jon Mason <jdmason@us.ibm.com> :
[snip]
>>adds a link down notification, and cleans up 
> 
> 
> Ok. I'll netif_msg it before someone else complains that the driver
> is too verbose.
[snip]

I started working on adding netif_msg checks to the printks. How far 
have you got? I don't want to duplicate your work.

Incidentally, is there a to-do list somewhere for the r8169 driver? 
Maybe I can help out with some of the things.

Thanks, bye, Rich =]

-- 
Richard Dawe [ http://homepages.nildram.co.uk/~phekda/richdawe/ ]

"You can't evaluate a man by logic alone."
   -- McCoy, "I, Mudd", Star Trek

Re: [PATCH 2/3] r8169: code clean-up

[Jon Mason]

On Sunday 20 February 2005 11:52 am, Richard Dawe wrote:
> Hello.
>
> Francois Romieu wrote:
> > Jon Mason <jdmason@us.ibm.com> :
>
> [snip]
>
> >>adds a link down notification, and cleans up
> >
> > Ok. I'll netif_msg it before someone else complains that the driver
> > is too verbose.
>
> [snip]
>
> I started working on adding netif_msg checks to the printks. How far
> have you got? I don't want to duplicate your work.
>
> Incidentally, is there a to-do list somewhere for the r8169 driver?

Not to my knowledge.  It would be nice if there was a wiki out there which 
lists all of the drivers and areas to work on (features enablement, open 
bugs, etc.)

> Maybe I can help out with some of the things.

I know Francois likes it when people test his patches.  :-)

> Thanks, bye, Rich =]

Re: [PATCH 2/3] r8169: code clean-up

[Francois Romieu]

Richard Dawe <rich@phekda.gotadsl.co.uk> :
[...]
> I started working on adding netif_msg checks to the printks. How far 
> have you got? I don't want to duplicate your work.

None so far. Go wild.

> Incidentally, is there a to-do list somewhere for the r8169 driver? 

(without priority)

- validate suspend/resume/wol

- fix the dac issues on x64
  Weird. Pita. Status needs to be updated.

- push jumbo frames beyond 7k
  see with Jon Mason

- merge with the 8139cp driver
  always interrupted but I have not given up

- fix the gcc 2.95.x miscompilations
  people upgrade their compiler and return to real life before it can be
  diagnosed. It should not be hard but I have not had the time to install
  a broken debian to reproduce it so far :o(

- benchmark/improve the performances
  Executive summary by Lennert Buytenhek: it sucks. If someone could identify
  where the bottleneck is with hard numbers...

- harvest the hopeless r8169 users on the web
  time consuming like hell but it really helps to figure if things are
  stable or not.

- backport to 2.4
  RSN. Pending queue is available here and will be pushed once 2.6.x is
  up-to-date.

- ask Ben Greear if he still experiences keyboard issues with its laptop
  when he uses the r8169 driver
  Pending for 5 months... Ahem.

> Maybe I can help out with some of the things.

Figure what's wrong with M. Gardiol setup ?

--
Ueimor

Re: [PATCH 2/3] r8169: code clean-up

[Jon Mason]

On Sunday 20 February 2005 06:28 pm, Francois Romieu wrote:
> Richard Dawe <rich@phekda.gotadsl.co.uk> :
> [...]
>
> > I started working on adding netif_msg checks to the printks. How far
> > have you got? I don't want to duplicate your work.
>
> None so far. Go wild.
>
> > Incidentally, is there a to-do list somewhere for the r8169 driver?
>
> (without priority)
>
> - validate suspend/resume/wol
>
> - fix the dac issues on x64
>   Weird. Pita. Status needs to be updated.
>
> - push jumbo frames beyond 7k
>   see with Jon Mason
>
> - merge with the 8139cp driver
>   always interrupted but I have not given up
>
> - fix the gcc 2.95.x miscompilations
>   people upgrade their compiler and return to real life before it can be
>   diagnosed. It should not be hard but I have not had the time to install
>   a broken debian to reproduce it so far :o(
>
> - benchmark/improve the performances
>   Executive summary by Lennert Buytenhek: it sucks. If someone could
> identify where the bottleneck is with hard numbers...
>
> - harvest the hopeless r8169 users on the web
>   time consuming like hell but it really helps to figure if things are
>   stable or not.
>
> - backport to 2.4
>   RSN. Pending queue is available here and will be pushed once 2.6.x is
>   up-to-date.
>
> - ask Ben Greear if he still experiences keyboard issues with its laptop
>   when he uses the r8169 driver
>   Pending for 5 months... Ahem.
>
> > Maybe I can help out with some of the things.
>
> Figure what's wrong with M. Gardiol setup ?

Fix ethtool statistics (amd64 only problem?)

Re: [PATCH 2/3] r8169: code clean-up

[Jon Mason]

On Saturday 19 February 2005 04:46 am, Francois Romieu wrote:
> Jon Mason <jdmason@us.ibm.com> :
> > Would you like me to rediff with the changes you suggested?
>
> I have done it and separated the changes. The printk stuff will go in
> a separate netif_msg_xxx and friends patch. See below if you have any
> question. Btw, you can Cc: netdev. It's interesting for everyone to know
> what's going on with r8169 devel.

netdev Cc'ed per your suggestion.

I've taken the liberity of removing all the patches, as they take up alot of 
space and look fine to me.  Good work.

[...]
> > Also, how does the jumbo frames patch look?
>
> 1 - Mangled.

I never said it was pretty, simply a working snapshot of my test driver.  I 
figured that I had been promising you this update for so long, I better send 
it out before you get upset with me.  ;-)

> 2 -
>
> @@ -1627,7 +1652,8 @@ out:
>  static inline void rtl8169_make_unusable_by_asic(struct RxDesc *desc)
>  {
>   desc->addr = 0x0badbadbadbadbadull;
> - desc->opts1 &= ~cpu_to_le32(DescOwn | RsvdMask);
> + desc->opts1 &= cpu_to_le32(RingEnd);
> + desc->opts2 = 0;
>  }
>
>  static void rtl8169_free_rx_skb(struct rtl8169_private *tp,
>
> Can you describe the scenario related to the change ?

The change to opts1 seems cleaner to me.  As the only the that the adapter 
needs to know is the RingEnd.  I can try removing it and see if it makes any 
difference.

Opts2 is VERY necessary to zero.  Without this zero, the adapter will change 
the partition point randomly of each packet (random because I have yet to 
determine the rhyme or reason).  I didn't want to spend too much time trying 
to determine the bahavior of this since I found out that zeroing it solved 
the problem.  If you can think of a reason why this doesn't work, I can 
always go back and try to figure it out.

> 3 -
>
> @@ -1652,6 +1680,7 @@ static inline void rtl8169_give_to_asic(
>  {
>   desc->addr = cpu_to_le64(mapping);
>   desc->opts1 |= cpu_to_le32(DescOwn + rx_buf_sz);
> + desc->opts2 = 0;
>  }
>
> I am curious to know if it's there because you noticed that the asic did
> not always update this word or because it is a good practice.
>
> Please note that in its current form it is racy: opts2 should be updated
> before opts1 and a memory barrier should be added.

See above for reason.  I can re-order the operations and a mb (rmd I would 
think).  

> 4 -
>
> @@ -2152,37 +2216,38 @@ rtl8169_rx_interrupt(struct net_device *
>    } else {
>     struct RxDesc *desc = tp->RxDescArray + entry;
>     struct sk_buff *skb = tp->Rx_skbuff[entry];
> -   int pkt_size = (status & 0x00001FFF) - 4;
> +   int pkt_size = (status & 0x00003FFF) - 4;
>     void (*pci_action)(struct pci_dev *, dma_addr_t,
>      size_t, int) = pci_dma_sync_single_for_device;
>
> -   rtl8169_rx_csum(skb, desc);
> -
>     pci_dma_sync_single_for_cpu(tp->pci_dev,
>      le64_to_cpu(desc->addr), tp->rx_buf_sz,
>      PCI_DMA_FROMDEVICE);
> -
> -   if (rtl8169_try_rx_copy(&skb, pkt_size, desc,
> -      tp->rx_buf_sz)) {
> +
> +   rtl8169_rx_csum(skb, desc);
>
> Why is rtl8169_rx_csum() moved around ?

Seemed to make more sense to me to have it after the 
pci_dma_sync_single_for_cpu.  If you disagree, I can move it back.  

> 5 -
> +   if (pkt_size > rx_copybreak) {
> +    memcpy(skb->data, sk_buff[0]->tail,
> +      tp->desc_part);
> +    skb_put(skb, tp->desc_part);
> +   } else {
> +    memcpy(skb->data, sk_buff[0]->tail, pkt_size);
> +    skb_put(skb, pkt_size);
> +   }
> +  } else
>
> I'd rather avoid the code duplication.

Me too, but I couldn't think of a better way.  Suggestions welcomed.

> 5 - rtl8169_return_to_asic() in rtl8169_rx_copy() seems suspect
>     when FirstFrag is true and the skb allocation failed.

In this case the desc is only a portion of the whole packet.  I thought it 
best to toss the whole packet if the driver couldn't alloc a skb, and return 
the descriptor so that it can be used again.  

Actually, talking about this gave me an idea.  Since desc_part is the largest 
skb we will ever need (aside from when they are combined), it would be more 
efficient to use that instead of the full size.  I will create a patch for 
this (and the mb changes) and resubmit it.  Let me know if you want me to 
return other things to their original spot.

> 6 - skb_put() ends duplicated. It should be possible to avoid it.

Is it?  I was specifically trying to aviod that.  Where do you see that?

Thanks,
Jon

Re: [PATCH 2/3] r8169: code clean-up

[Francois Romieu]

Jon Mason <jdmason@us.ibm.com> :
[...]
> I never said it was pretty, simply a working snapshot of my test driver.  I 
> figured that I had been promising you this update for so long, I better send 
> it out before you get upset with me.  ;-)

No, no, I meant mangled as in "kmail fcuked the tabs again".

[...]
> The change to opts1 seems cleaner to me.  As the only the that the adapter 
> needs to know is the RingEnd.  I can try removing it and see if it makes any 
> difference.

Until there is a stability or performance reason for it, I will not take
the change:
- it would add one extra parameter in any future problem report;
- neither me nor you can reproduce on it the testing done on the current form.

I would lazily label it as too much work with too low a priority.

> Opts2 is VERY necessary to zero.  Without this zero, the adapter will change 
> the partition point randomly of each packet (random because I have yet to 
> determine the rhyme or reason).  I didn't want to spend too much time trying 
> to determine the bahavior of this since I found out that zeroing it solved 
> the problem.  If you can think of a reason why this doesn't work, I can 
> always go back and try to figure it out.

Ok, this one really changes the behavior of the driver.

[...]
> See above for reason.  I can re-order the operations and a mb (rmd I would 
> think).  

So this one changes the behavior of the driver as well, right ?

The window may be narrow but I'll sleep more quietly if the ordering is
modified and the memory barrier added.

[...]
> > Why is rtl8169_rx_csum() moved around ?
> 
> Seemed to make more sense to me to have it after the 
> pci_dma_sync_single_for_cpu.  If you disagree, I can move it back.  

rtl8169_rx_csum() only uses the consistent DMA mapping so it does not
care.

[skb_put/memcpy duplication]
> 
> Me too, but I couldn't think of a better way.  Suggestions welcomed.

Nothing fancy. Something like:
	len  = (pkt_size > rx_copybreak) ? tp->desc_part : pkt_size;
	memcpy(skb->data, sk_buff[0]->tail, len);
	skb_put(skb, len);


> > 5 - rtl8169_return_to_asic() in rtl8169_rx_copy() seems suspect
> >     when FirstFrag is true and the skb allocation failed.
> 
> In this case the desc is only a portion of the whole packet.  I thought it 
> best to toss the whole packet if the driver couldn't alloc a skb, and return 
> the descriptor so that it can be used again.  

1 - the packet could be under rx_copybreak as well;
2 - the driver currently defers the loss of a packet until there is no memory
    pointed by the current descriptor used for Rx DMA from the adapter. If it
    is refilled soon enough, no one notices it.

> Actually, talking about this gave me an idea.  Since desc_part is the largest 
> skb we will ever need (aside from when they are combined), it would be more 
> efficient to use that instead of the full size.  I will create a patch for 
> this (and the mb changes) and resubmit it.  Let me know if you want me to 
> return other things to their original spot.

I am not sure I understand exactly what you mean. If you are saying that
the max size is known and can be allocated in advance so that only the
rx_copybreak calls for an instant allocation, we are saying the same thing
(actually this should have been point 3- above :o) ).

It will make my life easier if the patch only contains the minimal amount of
changes (easier reviewing, testing, confidence, blah blah, it's boring but
you got the idea).

> > 6 - skb_put() ends duplicated. It should be possible to avoid it.
> 
> Is it?  I was specifically trying to aviod that.  Where do you see that?

->
+   if (rtl8169_rx_copy(&skb, pkt_size, desc, tp)) {
     pci_action = pci_unmap_single;
     tp->Rx_skbuff[entry] = NULL;
+    skb_put(skb, pkt_size);
    }

-> rtl8169_rx_copy()
Lots of skb_put()

Don't bother too much about this one. I had expected to keep the
skb->dev = ...; skb_put(...); skb->proto = ... sequence as is to
minimize the changes but it is not necessarily doable/sane.

--
Ueimor

Re: [PATCH 2/3] r8169: code clean-up

[Jon Mason]

On Saturday 19 February 2005 04:30 pm, Francois Romieu wrote:
> Jon Mason <jdmason@us.ibm.com> :
> [...]
>
> > I never said it was pretty, simply a working snapshot of my test driver. 
> > I figured that I had been promising you this update for so long, I better
> > send it out before you get upset with me.  ;-)
>
> No, no, I meant mangled as in "kmail fcuked the tabs again".

Not 100% sure it is a kmail problem (as it could be my IMAP server).  I'll 
trying a different mail clent, and see if that makes any difference.  Thanks 
for the heads-up.

> [...]
>
> > The change to opts1 seems cleaner to me.  As the only the that the
> > adapter needs to know is the RingEnd.  I can try removing it and see if
> > it makes any difference.
>
> Until there is a stability or performance reason for it, I will not take
> the change:
> - it would add one extra parameter in any future problem report;
> - neither me nor you can reproduce on it the testing done on the current
> form.
>
> I would lazily label it as too much work with too low a priority.

Actually, I removed it and it causes the partition point to move, similiar to 
opts2.  I realize this isn't really an acceptable answer, so I will try to 
determine what is causing this weird behavior.

> > Opts2 is VERY necessary to zero.  Without this zero, the adapter will
> > change the partition point randomly of each packet (random because I have
> > yet to determine the rhyme or reason).  I didn't want to spend too much
> > time trying to determine the bahavior of this since I found out that
> > zeroing it solved the problem.  If you can think of a reason why this
> > doesn't work, I can always go back and try to figure it out.
>
> Ok, this one really changes the behavior of the driver.
>
> [...]
>
> > See above for reason.  I can re-order the operations and a mb (rmd I
> > would think).
>
> So this one changes the behavior of the driver as well, right ?
>
> The window may be narrow but I'll sleep more quietly if the ordering is
> modified and the memory barrier added.

fixed.  

> [...]
>
> > > Why is rtl8169_rx_csum() moved around ?
> >
> > Seemed to make more sense to me to have it after the
> > pci_dma_sync_single_for_cpu.  If you disagree, I can move it back.
>
> rtl8169_rx_csum() only uses the consistent DMA mapping so it does not
> care.

returned to its original spot.

> [skb_put/memcpy duplication]
>
> > Me too, but I couldn't think of a better way.  Suggestions welcomed.
>
> Nothing fancy. Something like:
>  len  = (pkt_size > rx_copybreak) ? tp->desc_part : pkt_size;
>  memcpy(skb->data, sk_buff[0]->tail, len);
>  skb_put(skb, len);

fixed.

> > > 5 - rtl8169_return_to_asic() in rtl8169_rx_copy() seems suspect
> > >     when FirstFrag is true and the skb allocation failed.
> >
> > In this case the desc is only a portion of the whole packet.  I thought
> > it best to toss the whole packet if the driver couldn't alloc a skb, and
> > return the descriptor so that it can be used again.
>
> 1 - the packet could be under rx_copybreak as well;
> 2 - the driver currently defers the loss of a packet until there is no
> memory pointed by the current descriptor used for Rx DMA from the adapter.
> If it is refilled soon enough, no one notices it.

True, but this problem existed before I made the jumbo frames changes.  The 
only way I can think to fix this is to do the following:

static inline int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
                              struct RxDesc *desc, struct rtl8169_private *tp)
{
        u32 status = le32_to_cpu(desc->opts1);

        if ((pkt_size > rx_copybreak) &&
                        ((status & FirstFrag) && (status & LastFrag)))
                return -1;

        if (status & FirstFrag) {
                struct sk_buff *skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
                u32 len = (pkt_size > rx_copybreak) ? tp->desc_part : 
pkt_size;

                if (skb) {
                        skb_reserve(skb, NET_IP_ALIGN);
                        memcpy(skb->data, sk_buff[0]->tail, len);
                        skb_put(skb, len);
                } else {
                        printk(KERN_INFO "no rx skb allocated\n");
   if (pkt_size <= rx_copybreak) 
    return -1;
                }

                tp->new_skb = skb;
        }

Is this acceptable?

> > Actually, talking about this gave me an idea.  Since desc_part is the
> > largest skb we will ever need (aside from when they are combined), it
> > would be more efficient to use that instead of the full size.  I will
> > create a patch for this (and the mb changes) and resubmit it.  Let me
> > know if you want me to return other things to their original spot.
>
> I am not sure I understand exactly what you mean. If you are saying that
> the max size is known and can be allocated in advance so that only the
> rx_copybreak calls for an instant allocation, we are saying the same thing
> (actually this should have been point 3- above :o) ).

No, I am saying that the max size is know for the skb rx ring and it is not 
rx_buf_sz, but desc_part.   I don't currently have this in the driver, but I 
can add it where driver allocs smaller skbs for jumbo frames enabled rx ring.  

If I am still not making sense, I can be more verbose.

> It will make my life easier if the patch only contains the minimal amount
> of changes (easier reviewing, testing, confidence, blah blah, it's boring
> but you got the idea).

Sorry, I guess I got a little over eager.  I will attempt to avoid this 
behavior in the future.  Forgiveness please.

> > > 6 - skb_put() ends duplicated. It should be possible to avoid it.
> >
> > Is it?  I was specifically trying to aviod that.  Where do you see that?
>
> ->
> +   if (rtl8169_rx_copy(&skb, pkt_size, desc, tp)) {
>      pci_action = pci_unmap_single;
>      tp->Rx_skbuff[entry] = NULL;
> +    skb_put(skb, pkt_size);
>     }
>
> -> rtl8169_rx_copy()
> Lots of skb_put()

Actually, I am using skb_put more frequently for a reason, and I probably need 
to explain why.  In rtl8169_rx_copy(), I am using it to keep track of the end 
of the skb, so that the driver knows the point to concatinate the next desc 
to the skb.  when it is all over, the tail points to the end of the skb and 
len is correct (which is the whole point of using skb_put), thereby removing 
the need to call it in the jumbo frames case.  

I realize this is being a little creative, but it is much cleaner code this 
way.  Otherwise the driver will need a global counter to use as a offset of 
the skb->tail and call skb_put after it is all over.  

> Don't bother too much about this one. I had expected to keep the
> skb->dev = ...; skb_put(...); skb->proto = ... sequence as is to
> minimize the changes but it is not necessarily doable/sane.
>
> --
> Ueimor

I will clean-up what I currently have with the changes you suggested and 
resend it.

Thanks,
Jon

Re: [PATCH 2/3] r8169: code clean-up

[Francois Romieu]

Jon Mason <jdmason@us.ibm.com> :
[...]
> Actually, I removed it and it causes the partition point to move, similiar to 
> opts2.  I realize this isn't really an acceptable answer, so I will try to 
> determine what is causing this weird behavior.

It is an acceptable answer: the behavior changes.

[...]
> True, but this problem existed before I made the jumbo frames changes.  The 

AFAIKS, if the rx_copybreak fails, be it because the packet is too big or
because the allocation failed, the current skb is sent to the upper layers.


> static inline int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
>                               struct RxDesc *desc, struct rtl8169_private *tp)
> {
>         u32 status = le32_to_cpu(desc->opts1);
> 
>         if ((pkt_size > rx_copybreak) &&
>                         ((status & FirstFrag) && (status & LastFrag)))
>                 return -1;
> 
>         if (status & FirstFrag) {
>                 struct sk_buff *skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
>                 u32 len = (pkt_size > rx_copybreak) ? tp->desc_part : pkt_size;
> 
>                 if (skb) {
>                         skb_reserve(skb, NET_IP_ALIGN);
>                         memcpy(skb->data, sk_buff[0]->tail, len);
>                         skb_put(skb, len);
>                 } else {
>                         printk(KERN_INFO "no rx skb allocated\n");
>                         if (pkt_size <= rx_copybreak) 
>                                 return -1;
>                 }
> 
>                 tp->new_skb = skb;
>         }
> 
> Is this acceptable?

I'll think more until I can figure I got the whole picture (only copy the
first frament ?). I would have expected something like the mess below:

static int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
			   struct RxDesc *desc, struct rtl8169_private *tp)
{
	u32 status = le32_to_cpu(desc->opts1);
	struct sk_buff *skb;
	int ret = -1;

	if ((pkt_size > rx_copybreak) && (status & FirstFrag) &&
	    (status & LastFrag)) {
		goto out;
	}

	if (pkt_size < rx_copybreak) {
		skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
		if (!skb)
			goto out;
		skb_reserve(skb, NET_IP_ALIGN);
	} else if (status & FirstFrag)
		skb = tp->new_skb = sk_buff[0];

	if ((pkt_size < rx_copybreak) || !(status & FirstFrag)) {
		memcpy(skb->data, sk_buff[0]->tail, pkt_size);
		rtl8169_return_to_asic(desc, tp->rx_buf_sz);
		*sk_buff = skb;
		ret = 0;
	}

	skb_put(skb, pkt_size);

out:
	return ret;
}

At this point I'd expect "How do you handle rtl8169_rx_copy() return ?" 
(or "yuck").

[...]
> If I am still not making sense, I can be more verbose.

Enlight me with the code, just to be sure.

[...]
> Sorry, I guess I got a little over eager.  I will attempt to avoid this 
> behavior in the future.  Forgiveness please.

No problem, really.

[...]
> I realize this is being a little creative, but it is much cleaner code this 
> way. Otherwise the driver will need a global counter to use as a offset of 
> the skb->tail and call skb_put after it is all over.

Ok, ok, I did not get it in the original patch.

--
Ueimor

Re: [PATCH 2/3] r8169: code clean-up

[Jon Mason]

On Sunday 20 February 2005 05:34 pm, Francois Romieu wrote:
[...]
> > True, but this problem existed before I made the jumbo frames changes.  The 
> 
> AFAIKS, if the rx_copybreak fails, be it because the packet is too big or
> because the allocation failed, the current skb is sent to the upper layers.

You are correct, I misunderstood the error path of the previous rx_copy routine.  

> > static inline int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
> >                               struct RxDesc *desc, struct rtl8169_private *tp)
> > {
> >         u32 status = le32_to_cpu(desc->opts1);
> > 
> >         if ((pkt_size > rx_copybreak) &&
> >                         ((status & FirstFrag) && (status & LastFrag)))
> >                 return -1;
> > 
> >         if (status & FirstFrag) {
> >                 struct sk_buff *skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
> >                 u32 len = (pkt_size > rx_copybreak) ? tp->desc_part : pkt_size;
> > 
> >                 if (skb) {
> >                         skb_reserve(skb, NET_IP_ALIGN);
> >                         memcpy(skb->data, sk_buff[0]->tail, len);
> >                         skb_put(skb, len);
> >                 } else {
> >                         printk(KERN_INFO "no rx skb allocated\n");
> >                         if (pkt_size <= rx_copybreak) 
> >                                 return -1;
> >                 }
> > 
> >                 tp->new_skb = skb;
> >         }
> > 
> > Is this acceptable?
> 
> I'll think more until I can figure I got the whole picture (only copy the
> first frament ?). I would have expected something like the mess below:
> 
> static int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
>       struct RxDesc *desc, struct rtl8169_private *tp)
> {
>  u32 status = le32_to_cpu(desc->opts1);
>  struct sk_buff *skb;
>  int ret = -1;
> 
>  if ((pkt_size > rx_copybreak) && (status & FirstFrag) &&
>      (status & LastFrag)) {
>   goto out;
>  }
> 
>  if (pkt_size < rx_copybreak) {
>   skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
>   if (!skb)
>    goto out;
>   skb_reserve(skb, NET_IP_ALIGN);
>  } else if (status & FirstFrag)
>   skb = tp->new_skb = sk_buff[0];
> 
>  if ((pkt_size < rx_copybreak) || !(status & FirstFrag)) {
>   memcpy(skb->data, sk_buff[0]->tail, pkt_size);
>   rtl8169_return_to_asic(desc, tp->rx_buf_sz);
>   *sk_buff = skb;
>   ret = 0;
>  }
> 
>  skb_put(skb, pkt_size);
> 
> out:
>  return ret;
> }
> 
> At this point I'd expect "How do you handle rtl8169_rx_copy() return ?" 
> (or "yuck").

My main problem with the above patch is that is assumes that there will be a maximum of 2 descriptors per jumbo frame (which isn't the case).  This might have been caused by my incomplete patch given above.  This is what I currently have:
static inline int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
                                struct RxDesc *desc, struct rtl8169_private *tp)
{
        u32 status = le32_to_cpu(desc->opts1);

        if ((pkt_size > rx_copybreak) &&
                        ((status & FirstFrag) && (status & LastFrag)))
                return -1;

        if (status & FirstFrag) {
                u32 len = (pkt_size > rx_copybreak) ? tp->desc_part : pkt_size;
                struct sk_buff *skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
                if (skb) {
                        skb_reserve(skb, NET_IP_ALIGN);
                        memcpy(skb->data, sk_buff[0]->tail, len);
                        skb_put(skb, len);
                } else {
                        printk(KERN_INFO "no rx skb allocated\n");
                        if (pkt_size <= rx_copybreak)
                                return -1;
                }

                tp->new_skb = skb;
        }

        if (!(status & FirstFrag) && !(status & LastFrag) && tp->new_skb) {
                memcpy(tp->new_skb->tail, sk_buff[0]->tail, tp->desc_part);
                skb_put(tp->new_skb, tp->desc_part);
        }

        if (status & LastFrag) {
                if (pkt_size > rx_copybreak && tp->new_skb) {
                        memcpy(tp->new_skb->tail, sk_buff[0]->tail,
                                        pkt_size - tp->new_skb->len);

                        skb_put(tp->new_skb, pkt_size - tp->new_skb->len);
                }

                *sk_buff = tp->new_skb;
        }

        rtl8169_return_to_asic(desc, tp->rx_buf_sz);

        return 0;
}



> [...]
> > If I am still not making sense, I can be more verbose.
> 
> Enlight me with the code, just to be sure.

Working (but still hackish) patch:
--- drivers/net/r8169.c.0220    2005-02-20 22:45:07.000000000 -0600
+++ drivers/net/r8169.c 2005-02-20 22:43:27.000000000 -0600
@@ -1663,7 +1663,8 @@ static void rtl8169_free_rx_skb(struct r
 {
        struct pci_dev *pdev = tp->pci_dev;

-       pci_unmap_single(pdev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
+       //pci_unmap_single(pdev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
+       pci_unmap_single(pdev, le64_to_cpu(desc->addr), tp->desc_part+ETH_HLEN,
                         PCI_DMA_FROMDEVICE);
        dev_kfree_skb(*sk_buff);
        *sk_buff = NULL;
@@ -1694,14 +1695,14 @@ static int rtl8169_alloc_rx_skb(struct r
        dma_addr_t mapping;
        int ret = 0;

-       skb = dev_alloc_skb(rx_buf_sz + NET_IP_ALIGN);
+       skb = dev_alloc_skb(tp->desc_part + ETH_HLEN + NET_IP_ALIGN);
        if (!skb)
                goto err_out;

        skb_reserve(skb, NET_IP_ALIGN);
        *sk_buff = skb;

-       mapping = pci_map_single(tp->pci_dev, skb->tail, rx_buf_sz,
+       mapping = pci_map_single(tp->pci_dev, skb->tail, tp->desc_part+ETH_HLEN,
                                 PCI_DMA_FROMDEVICE);

        rtl8169_give_to_asic(desc, mapping, rx_buf_sz);
@@ -2225,7 +2226,7 @@ rtl8169_rx_interrupt(struct net_device *
                        rtl8169_rx_csum(skb, desc);

                        pci_dma_sync_single_for_cpu(tp->pci_dev,
-                               le64_to_cpu(desc->addr), tp->rx_buf_sz,
+                               le64_to_cpu(desc->addr), tp->desc_part+ETH_HLEN,
                                PCI_DMA_FROMDEVICE);

                        if (rtl8169_rx_copy(&skb, pkt_size, desc, tp)) {
@@ -2235,7 +2236,7 @@ rtl8169_rx_interrupt(struct net_device *
                        }

                        pci_action(tp->pci_dev, le64_to_cpu(desc->addr),
-                                  tp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+                                  tp->desc_part+ETH_HLEN, PCI_DMA_FROMDEVICE);

                        if (skb && (status & LastFrag)) {
                                skb->dev = dev;


A more complete patch to follow shortly (as I want to try and fix my tabs problem).

Re: [PATCH 2/3] r8169: code clean-up

[Francois Romieu]

Jon Mason <jdmason@us.ibm.com> :
[...]
> My main problem with the above patch is that is assumes that there will
> be a maximum of 2 descriptors per jumbo frame (which isn't the case). 

Where do you see such an assumption ?

--
Ueimor