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

[Francois Romieu <romieu@fr.zoreil.com>]

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