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

[Steve Wise]

On Thu, 2007-02-01 at 21:22 +0200, Michael S. Tsirkin wrote:
> > > 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.
> 
> Not sure how do you mean all this. You do kfree_skb immediately in the
> arp processing function. Will this not call the destructor directly?
> 

No because the skb refcnt gets bumped by the dev packet code before
passing it up to each snoop function.  So the destructor fn will get
called only when the _last_ user of this skbuf frees it.  If by some
reason we are the last ref, then yes, we'd get called immediately.  But
that's not what happens because the snoopers get added to the end of the
list of users who want any given ethertype packet.  Hope that makes
sense.

> Anyway, it seems too risky to change the code a lot now.
> what I am concerned is that this could have broken working code.
> 

I tested it with IB and iWARP.

> To reduce the risk of problems for existing code,
> I'd like to see something like the following:
> 
> 	if (someone asked for notification on neighbour changes)
> 		do the destructor trick
> 
> 	if (someone asked for notification on address resolution)
> 		call the destructor directly
> 
> Could you code this up please?

There's no easy way to tell who asked for notifications. And
particularly why they asked for notification.

I think we should leave it as-is.  If we have problems, we'll fix it.

Or you could put your arp snoop code back in addr.c and address
translation will not use netevents.  But still thing we should leave
it...