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. _______________________________________________ openib-general mailing list [email protected] http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
