[openib-general] [PATCH 00/12] ofed_1_2 - Neighbour update support

[Steve Wise]

On Thu, 2007-02-01 at 14:19 +0200, Michael S. Tsirkin wrote:
> > Here are the backports for snooping arp packets to generate neighbour
> > update netevents.
> 
> OK, I went (somewhat belatedly) over this code in more depth and I see
> a couple of issues that I'd like you to address:
> 
> - There's some trailing whitespace in some netevet.c files.
>   Could you clean these please?
> 

You took care of these I assume based on your followup email.

> - I see:
> 	$ diff ./kernel_addons/backport/2.6.9_U4/include/src/netevent.c
> 	kernel_addons/backport/2.6.5_sles9_sp3/include/src/netevent.c
> 	> #include <linux/skbuff.h>
> 
> Should not redhat backports include skbuff.h too?
> They do use skbuff struct so it seems it is cleaner to include
> directly, and we would get identical code for redhat and suse.
>  

Yup.

> - What is the reason for:
>         if ((op == ARPOP_REQUEST || op == ARPOP_REPLY) && !skb->destructor)
> 	                skb->destructor = destructor;
> 
> 	kfree_skb(skb);
> 
> Could we miss events because skb has a desctructor?

Yes.  I looked through the ethernet drivers and didn't see anyone using
destructors.  I thought perhaps this is ok for backports.  There are
ways to address this issue:

1) Enhance the current code to save off the original destructor function
if it exists and put in ours.  Then when our function is called, we do
our processing, then call the original destructor function.  We would
need to save the original function ptr somewhere. 

2) schedule the function to happen at a later time and hope the ARP
subsystem has already updated the neigh table.  I opted against this
approach because it doesn't ensure that the neigh entry was updated
before we act on it.

> Can we just call the descructor function directly (this is what addr.c
> did previously, and this apparently worked fine).

The original addr.c snoop code worked fine for IB address resolution and
for the initial ARP resolution for iWARP devices, but not for notifying
iWARP devices when a neighbour changes.  For instance, if the neighbour
mac address changes, then the iWARP device needs to be notified so it
can update its L2 table maintained in the device. 

We need to defer calling the destructor function until the ARP subsystem
has processed this ARP packet.  Through testing, I saw that our snoop
function gets called _before_ the ARP subsystem processes the ARP
packet.  So the neighbour entry hasn't been updated yet.  Hooking via
destructor calls our function _after_ the ARP subsystem has updated the
neighbour.  So we can then lookup the neigh entry and do the callouts.