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

2007-02-04 Thread Michael S. Tsirkin 
 Quoting Steve Wise [EMAIL PROTECTED]:
 Subject: Re: [PATCH 00/12] ofed_1_2 - Neighbour update support
 
 On Fri, 2007-02-02 at 08:03 +0200, Michael S. Tsirkin wrote:
   We could use a global refcnt to count the number of pending destructions
   and use a completion object to block unload until all the destructors
   fire and the refcnt goes to zero.
  
  It has the same race as module refcnt. So just use that.
  
 
 I don't understand the race.  Can you explain please?  This should be
 able to be done without a race with a refcnt, a spinlock, a bit saying
 we're unloading, and a completion object.
 
 But maybe I'm confused ;-)

In short, the rule is that you can't pass a pointer to your function
to another module, and the unload module safely without synchronizing with that
other module.

Simplified example:

destructor
{
complete(foo);
A:
return;
}

module_cleanup:
{
wait(foo)
return;
}

Now, assume destructor runs up to point A, then your module unloads,
and the memory its text occupied is overwritten by something else.
An attempt to execute code from point A will now crash.
So completion is not better than just module refcount here.

That said, I think the race is unlikely and just using module
refcount should be sufficient, and it's certainly simple.

-- 
MST

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-04 Thread Michael S. Tsirkin 
 If you're worried about regressing straight rdma address
 translation, then you can call the address translation timer function
 synchronously in the snoop function like before and change the
 addr_trans module to not use netevents...

This seems the prudent thing to do.
OK, I'll do that.

-- 
MST

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-02 Thread Steve Wise 
On Fri, 2007-02-02 at 08:03 +0200, Michael S. Tsirkin wrote:
  We could use a global refcnt to count the number of pending destructions
  and use a completion object to block unload until all the destructors
  fire and the refcnt goes to zero.
 
 It has the same race as module refcnt. So just use that.
 

I don't understand the race.  Can you explain please?  This should be
able to be done without a race with a refcnt, a spinlock, a bit saying
we're unloading, and a completion object.

But maybe I'm confused ;-)




___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Michael S. Tsirkin 
 Quoting Steve Wise [EMAIL PROTECTED]:
 Subject: [PATCH 00/12] ofed_1_2 - Neighbour update support
 
 
 Michael/Vlad:
 
 Here are the backports for snooping arp packets to generate neighbour
 update netevents.  Also included is the addr.c patch to act on all valid
 neigh update events.  If this series looks good to you then I'll push
 this up and you all can pull it from my git tree.

This patches seems to have created a reference leak on each neighbour
as a result ipoib interface could not be brought down.
It also seems that RHASU2 backport was missing code.
I pushed out the following:


commit d140398db0da0beb3172e0ccf14ef3023cafec9c
Author: Michael S. Tsirkin [EMAIL PROTECTED]
Date:   Thu Feb 1 12:21:34 2007 +0200

Fix neighbour reference leak in netevent.c

Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]

diff --git a/kernel_addons/backport/2.6.11/include/src/netevent.c 
b/kernel_addons/backport/2.6.11/include/src/netevent.c
index 6a8df29..0d26662 100644
--- a/kernel_addons/backport/2.6.11/include/src/netevent.c
+++ b/kernel_addons/backport/2.6.11/include/src/netevent.c
@@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
memcpy(gw, arp_ptr, 4);
n = neigh_lookup(arp_tbl, gw, skb-dev);
-   if (n)
+   if (n) {
call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
+   neigh_release(n);
+   }
return;
 }
 
diff --git a/kernel_addons/backport/2.6.12/include/src/netevent.c 
b/kernel_addons/backport/2.6.12/include/src/netevent.c
index 6a8df29..0d26662 100644
--- a/kernel_addons/backport/2.6.12/include/src/netevent.c
+++ b/kernel_addons/backport/2.6.12/include/src/netevent.c
@@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
memcpy(gw, arp_ptr, 4);
n = neigh_lookup(arp_tbl, gw, skb-dev);
-   if (n)
+   if (n) {
call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
+   neigh_release(n);
+   }
return;
 }
 
diff --git a/kernel_addons/backport/2.6.13/include/src/netevent.c 
b/kernel_addons/backport/2.6.13/include/src/netevent.c
index 6a8df29..0d26662 100644
--- a/kernel_addons/backport/2.6.13/include/src/netevent.c
+++ b/kernel_addons/backport/2.6.13/include/src/netevent.c
@@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
memcpy(gw, arp_ptr, 4);
n = neigh_lookup(arp_tbl, gw, skb-dev);
-   if (n)
+   if (n) {
call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
+   neigh_release(n);
+   }
return;
 }
 
diff --git a/kernel_addons/backport/2.6.14/include/src/netevent.c 
b/kernel_addons/backport/2.6.14/include/src/netevent.c
index 188283c..17a12ff 100644
--- a/kernel_addons/backport/2.6.14/include/src/netevent.c
+++ b/kernel_addons/backport/2.6.14/include/src/netevent.c
@@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
memcpy(gw, arp_ptr, 4);
n = neigh_lookup(arp_tbl, gw, skb-dev);
-   if (n)
+   if (n) {
call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
+   neigh_release(n);
+   }
return;
 }
 
diff --git a/kernel_addons/backport/2.6.15/include/src/netevent.c 
b/kernel_addons/backport/2.6.15/include/src/netevent.c
index 188283c..17a12ff 100644
--- a/kernel_addons/backport/2.6.15/include/src/netevent.c
+++ b/kernel_addons/backport/2.6.15/include/src/netevent.c
@@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
memcpy(gw, arp_ptr, 4);
n = neigh_lookup(arp_tbl, gw, skb-dev);
-   if (n)
+   if (n) {
call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
+   neigh_release(n);
+   }
return;
 }
 
diff --git a/kernel_addons/backport/2.6.15_ubuntu606/include/src/netevent.c 
b/kernel_addons/backport/2.6.15_ubuntu606/include/src/netevent.c
index 188283c..17a12ff 100644
--- a/kernel_addons/backport/2.6.15_ubuntu606/include/src/netevent.c
+++ b/kernel_addons/backport/2.6.15_ubuntu606/include/src/netevent.c
@@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
memcpy(gw, arp_ptr, 4);
n = neigh_lookup(arp_tbl, gw, skb-dev);
-   if (n)
+   if (n) {
call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
+   neigh_release(n);
+   }
return;
 }
 
diff --git a/kernel_addons/backport/2.6.16/include/src/netevent.c 
b/kernel_addons/backport/2.6.16/include/src/netevent.c
index 188283c..17a12ff 100644
--- a/kernel_addons/backport/2.6.16/include/src/netevent.c
+++

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

2007-02-01 Thread Michael S. Tsirkin 
 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?

- 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.
 
- 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?
Can we just call the descructor function directly (this is what addr.c
did previously, and this apparently worked fine).

Steve, could you pls clone ofed git and address these?


-- 
MST

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Michael S. Tsirkin 
 - There's some trailing whitespace in some netevet.c files.
   Could you clean these please?

OK, fixed the trailing whitespace and pushed out.

-- 
MST

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Steve Wise 
Looks good.

Thanks,

Steve.


On Thu, 2007-02-01 at 14:10 +0200, Michael S. Tsirkin wrote:
  Quoting Steve Wise [EMAIL PROTECTED]:
  Subject: [PATCH 00/12] ofed_1_2 - Neighbour update support
  
  
  Michael/Vlad:
  
  Here are the backports for snooping arp packets to generate neighbour
  update netevents.  Also included is the addr.c patch to act on all valid
  neigh update events.  If this series looks good to you then I'll push
  this up and you all can pull it from my git tree.
 
 This patches seems to have created a reference leak on each neighbour
 as a result ipoib interface could not be brought down.
 It also seems that RHASU2 backport was missing code.
 I pushed out the following:
 
 
 commit d140398db0da0beb3172e0ccf14ef3023cafec9c
 Author: Michael S. Tsirkin [EMAIL PROTECTED]
 Date:   Thu Feb 1 12:21:34 2007 +0200
 
 Fix neighbour reference leak in netevent.c
 
 Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]
 
 diff --git a/kernel_addons/backport/2.6.11/include/src/netevent.c 
 b/kernel_addons/backport/2.6.11/include/src/netevent.c
 index 6a8df29..0d26662 100644
 --- a/kernel_addons/backport/2.6.11/include/src/netevent.c
 +++ b/kernel_addons/backport/2.6.11/include/src/netevent.c
 @@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
   arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
   memcpy(gw, arp_ptr, 4);
   n = neigh_lookup(arp_tbl, gw, skb-dev);
 - if (n)
 + if (n) {
   call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
 + neigh_release(n);
 + }
   return;
  }
  
 diff --git a/kernel_addons/backport/2.6.12/include/src/netevent.c 
 b/kernel_addons/backport/2.6.12/include/src/netevent.c
 index 6a8df29..0d26662 100644
 --- a/kernel_addons/backport/2.6.12/include/src/netevent.c
 +++ b/kernel_addons/backport/2.6.12/include/src/netevent.c
 @@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
   arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
   memcpy(gw, arp_ptr, 4);
   n = neigh_lookup(arp_tbl, gw, skb-dev);
 - if (n)
 + if (n) {
   call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
 + neigh_release(n);
 + }
   return;
  }
  
 diff --git a/kernel_addons/backport/2.6.13/include/src/netevent.c 
 b/kernel_addons/backport/2.6.13/include/src/netevent.c
 index 6a8df29..0d26662 100644
 --- a/kernel_addons/backport/2.6.13/include/src/netevent.c
 +++ b/kernel_addons/backport/2.6.13/include/src/netevent.c
 @@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
   arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
   memcpy(gw, arp_ptr, 4);
   n = neigh_lookup(arp_tbl, gw, skb-dev);
 - if (n)
 + if (n) {
   call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
 + neigh_release(n);
 + }
   return;
  }
  
 diff --git a/kernel_addons/backport/2.6.14/include/src/netevent.c 
 b/kernel_addons/backport/2.6.14/include/src/netevent.c
 index 188283c..17a12ff 100644
 --- a/kernel_addons/backport/2.6.14/include/src/netevent.c
 +++ b/kernel_addons/backport/2.6.14/include/src/netevent.c
 @@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
   arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
   memcpy(gw, arp_ptr, 4);
   n = neigh_lookup(arp_tbl, gw, skb-dev);
 - if (n)
 + if (n) {
   call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
 + neigh_release(n);
 + }
   return;
  }
  
 diff --git a/kernel_addons/backport/2.6.15/include/src/netevent.c 
 b/kernel_addons/backport/2.6.15/include/src/netevent.c
 index 188283c..17a12ff 100644
 --- a/kernel_addons/backport/2.6.15/include/src/netevent.c
 +++ b/kernel_addons/backport/2.6.15/include/src/netevent.c
 @@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
   arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
   memcpy(gw, arp_ptr, 4);
   n = neigh_lookup(arp_tbl, gw, skb-dev);
 - if (n)
 + if (n) {
   call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
 + neigh_release(n);
 + }
   return;
  }
  
 diff --git a/kernel_addons/backport/2.6.15_ubuntu606/include/src/netevent.c 
 b/kernel_addons/backport/2.6.15_ubuntu606/include/src/netevent.c
 index 188283c..17a12ff 100644
 --- a/kernel_addons/backport/2.6.15_ubuntu606/include/src/netevent.c
 +++ b/kernel_addons/backport/2.6.15_ubuntu606/include/src/netevent.c
 @@ -38,8 +38,10 @@ static void destructor(struct sk_buff *skb)
   arp_ptr = skb-nh.raw + sizeof(struct arphdr) + skb-dev-addr_len;
   memcpy(gw, arp_ptr, 4);
   n = neigh_lookup(arp_tbl, gw, skb-dev);
 - if (n)
 + if (n) {
   call_netevent_notifiers(NETEVENT_NEIGH_UPDATE, n);
 + neigh_release(n);
 + }
   return;
  }
  
 diff --git a/kernel_addons/backport/2.6.16/include/src/netevent.c

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

2007-02-01 Thread 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.



___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Michael S. Tsirkin 
  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?

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

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?

-- 
MST

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread 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...








___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Michael S. Tsirkin 
 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...

I think the issues need to be addressed in some way.

I think I see another issue with the destructor approach: ib_core could
be unloaded while skb with destructor pointing to our code is still around.
This will lead to nasty crashes without clear backtrace on screen if text
segment memory gets over-written and the destructor gets called afterwards.

It currently seems that invoking the callback function directly rather than
sticking it in skb-destructor is the lesser of evils at this point.
But I'll think all this over, and I'd like to ask you to do this too,
and post some suggestions.

I can think of some more complicated approaches that might work better
for iwarp. Off the top of my head, our netevents implementation could
keep a reference on the skb, start a timer, check the users counter on skb and
call the notifier chain when it drops to 1. Let's sleep on it.

-- 
MST

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Steve Wise 
On Fri, 2007-02-02 at 00:24 +0200, Michael S. Tsirkin wrote:
  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...
 
 I think the issues need to be addressed in some way.
 
 I think I see another issue with the destructor approach: ib_core could
 be unloaded while skb with destructor pointing to our code is still around.
 This will lead to nasty crashes without clear backtrace on screen if text
 segment memory gets over-written and the destructor gets called afterwards.
 

Yes...hmm...  We could reference the module in the snoop function and
deref it in the destructor function.

 It currently seems that invoking the callback function directly rather than
 sticking it in skb-destructor is the lesser of evils at this point.
 But I'll think all this over, and I'd like to ask you to do this too,
 and post some suggestions.
 

Ok.

 I can think of some more complicated approaches that might work better
 for iwarp. Off the top of my head, our netevents implementation could
 keep a reference on the skb, start a timer, check the users counter on skb and
 call the notifier chain when it drops to 1. Let's sleep on it.
 

Ok.  I'll ponder it some more.  But we could solve the module unload
issue via module refs methinks.


Steve.
 


___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Michael S. Tsirkin 
  I can think of some more complicated approaches that might work better
  for iwarp. Off the top of my head, our netevents implementation could
  keep a reference on the skb, start a timer, check the users counter on skb 
  and
  call the notifier chain when it drops to 1. Let's sleep on it.
  
 
 Ok.  I'll ponder it some more.  But we could solve the module unload
 issue via module refs methinks.

This almost never works cleanly - module can't reference itself
without races: module can get unloaded after it drops the reference
to itself and before the function exits.
But I agree such a race is mostly theoretical.

And we still have the case where destructor != NULL.

Certainly something to think about.

-- 
MST

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Steve Wise 
On Fri, 2007-02-02 at 00:48 +0200, Michael S. Tsirkin wrote:
   I can think of some more complicated approaches that might work better
   for iwarp. Off the top of my head, our netevents implementation could
   keep a reference on the skb, start a timer, check the users counter on 
   skb and
   call the notifier chain when it drops to 1. Let's sleep on it.
   

Remembering which skbs to check later requires more complication.  Here
is one method to handle this and do what you suggest above.

In the snoop function:

Clone the skb and save the original skb ptr in the new skb-cb area.
This area is ours to use on a freshly cloned skbuff.  Add this new skb
ptr to a linked list of outstanding netevents to be processed later.
Don't free the original skb passed in.  This keeps the reference on it
like you proposed above.  Schedule a delayed work handler for a few
ticks in the future.

In the delayed work handler:

Walk the pending netevents skb list.  For each pending skb, get the
original skb ptr from the cloned skb-cb area, and if the user count is
now 1 then do the current destructor() logic, remove the skb from the
pending list, and free both skbs.  If the list is not empty reschedule
the delayed work handler for a few ticks later.

In the module unload function:

cancel any delayed work handling
walk the pending list and free the skbs and the original snooped skbs.

This solves the destructor issue and the rmmod issue, but is more
complicated.  If you're worried about regressing straight rdma address
translation, then you can call the address translation timer function
synchronously in the snoop function like before and change the
addr_trans module to not use netevents...


Steve.
 




___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Michael S. Tsirkin 
 Quoting Steve Wise [EMAIL PROTECTED]:
 Subject: Re: [PATCH 00/12] ofed_1_2 - Neighbour update support
 
 On Fri, 2007-02-02 at 00:48 +0200, Michael S. Tsirkin wrote:
I can think of some more complicated approaches that might work better
for iwarp. Off the top of my head, our netevents implementation could
keep a reference on the skb, start a timer, check the users counter on 
skb and
call the notifier chain when it drops to 1. Let's sleep on it.

 
 Remembering which skbs to check later requires more complication.  Here
 is one method to handle this and do what you suggest above.
 
 In the snoop function:
 
 Clone the skb and save the original skb ptr in the new skb-cb area.
 This area is ours to use on a freshly cloned skbuff.  Add this new skb
 ptr to a linked list of outstanding netevents to be processed later.
 Don't free the original skb passed in.  This keeps the reference on it
 like you proposed above.  Schedule a delayed work handler for a few
 ticks in the future.
 
 In the delayed work handler:
 
 Walk the pending netevents skb list.  For each pending skb, get the
 original skb ptr from the cloned skb-cb area, and if the user count is
 now 1 then do the current destructor() logic, remove the skb from the
 pending list, and free both skbs.  If the list is not empty reschedule
 the delayed work handler for a few ticks later.
 
 In the module unload function:
 
 cancel any delayed work handling
 walk the pending list and free the skbs and the original snooped skbs.
 
 This solves the destructor issue and the rmmod issue, but is more
 complicated.  If you're worried about regressing straight rdma address
 translation, then you can call the address translation timer function
 synchronously in the snoop function like before and change the
 addr_trans module to not use netevents...


Yes, this is what I proposed above. It does all sound quite complicated.
Some notes:
- you don't need an skb just too keep a void*. create your own
  structure for this.
- better use a timer than a workqueue - you are calling netevents
  from atomic context on new kernels anyway.

So maybe destructor with module ref counting is better.
Donnu.

-- 
MST

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-02-01 Thread Steve Wise 
On Fri, 2007-02-02 at 01:33 +0200, Michael S. Tsirkin wrote:
  Quoting Steve Wise [EMAIL PROTECTED]:
  Subject: Re: [PATCH 00/12] ofed_1_2 - Neighbour update support
  
  On Fri, 2007-02-02 at 00:48 +0200, Michael S. Tsirkin wrote:
 I can think of some more complicated approaches that might work better
 for iwarp. Off the top of my head, our netevents implementation could
 keep a reference on the skb, start a timer, check the users counter 
 on skb and
 call the notifier chain when it drops to 1. Let's sleep on it.
 
  
  Remembering which skbs to check later requires more complication.  Here
  is one method to handle this and do what you suggest above.
  
  In the snoop function:
  
  Clone the skb and save the original skb ptr in the new skb-cb area.
  This area is ours to use on a freshly cloned skbuff.  Add this new skb
  ptr to a linked list of outstanding netevents to be processed later.
  Don't free the original skb passed in.  This keeps the reference on it
  like you proposed above.  Schedule a delayed work handler for a few
  ticks in the future.
  
  In the delayed work handler:
  
  Walk the pending netevents skb list.  For each pending skb, get the
  original skb ptr from the cloned skb-cb area, and if the user count is
  now 1 then do the current destructor() logic, remove the skb from the
  pending list, and free both skbs.  If the list is not empty reschedule
  the delayed work handler for a few ticks later.
  
  In the module unload function:
  
  cancel any delayed work handling
  walk the pending list and free the skbs and the original snooped skbs.
  
  This solves the destructor issue and the rmmod issue, but is more
  complicated.  If you're worried about regressing straight rdma address
  translation, then you can call the address translation timer function
  synchronously in the snoop function like before and change the
  addr_trans module to not use netevents...
 
 
 Yes, this is what I proposed above. It does all sound quite complicated.
 Some notes:
   - you don't need an skb just too keep a void*. create your own
 structure for this.
   - better use a timer than a workqueue - you are calling netevents
 from atomic context on new kernels anyway.
 
 So maybe destructor with module ref counting is better.
 Donnu.

We could use a global refcnt to count the number of pending destructions
and use a completion object to block unload until all the destructors
fire and the refcnt goes to zero.






___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-01-30 Thread Vladimir Sokolovsky 
On Fri, 2007-01-26 at 15:11 -0600, Steve Wise wrote:
 Michael/Vlad:
 
 I've pushed these up to my git tree.  Can you merge them in?  
 
 git://staging.openfabrics.com/~swise/ofed_1_2.git cxgb3
 
 Here is the short log of the commits:
 
 aedc0b3c1681fb550ec4b8d1021caa2ce3dcbfd7 iw_cxgb3: allow doorbell mappings 
 with VM_READ set.
 5ea83b9e3ec6f9c74040944adb83e4faf6613fe1 Backport Chelsio to rhel5 
 (2.6.18_FC6).
 ff38246f6f07ff25609eaa304a707748904bf2bf Backport sles9sp3: Simulate neigh 
 update events by snooping ARP packets
 b88d46d10ce15f8ee725454f4998af6497cc13e1 Backport rhel4u4: Simulate neigh 
 update events by snooping ARP packets
 ab3a817b10da2df2e3d5bf08018be3d0212dc5bd Backport 2.6.11: Simulate neigh 
 update events by snooping ARP packets
 e545001a94c180c32b8b15d4ca4351506bd50fc2 Backport 2.6.12: Simulate neigh 
 update events by snooping ARP packets
 8ddafe035c1a997c7625ae1bd42767deed148cb7 Backport 2.6.13: Simulate neigh 
 update events by snooping ARP packets
 ef260b8242d90edcabdc3153b829eda65d451672 Backport 2.6.14: Simulate neigh 
 update events by snooping ARP packets
 84c78965a7c6a2a831fb2a49c6936321e2566904 Backport ubuntu606: Simulate neigh 
 update events by snooping ARP packets
 eb09f52a33471613fc29f898dfad8d9a57238d3e Backport 2.6.15: Simulate neigh 
 update events by snooping ARP packets
 ddc3ec432bd1898005ab52241d125dd4a71436aa Backport sles10: Simulate neigh 
 update events by snooping ARP packets
 b4af429744ff06545b2941fc5ef1ab4d6f0c0e77 Backport 2.6.16: Simulate neigh 
 update events by snooping ARP packets
 fe1a597f3aa409465d5b1b577a3b28c4a002f143 Backport 2.6.17: Simulate neighbour 
 update events by snooping ARP packets
 9b3bfe5696aa417d38ce903eb345a03d65743dd2 Handle Ethernet neighbour updates 
 during route resolution.
 

Done.

-- 
Vladimir Sokolovsky [EMAIL PROTECTED]
Mellanox Technologies Ltd.

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-01-26 Thread Steve Wise 

Shall I push these up for you to pull from my git tree?

On Thu, 2007-01-25 at 13:13 -0600, Steve Wise wrote:
 Michael/Vlad:
 
 Here are the backports for snooping arp packets to generate neighbour
 update netevents.  Also included is the addr.c patch to act on all valid
 neigh update events.  If this series looks good to you then I'll push
 this up and you all can pull it from my git tree.
 
 
 Steve.
 
 ___
 openib-general mailing list
 openib-general@openib.org
 http://openib.org/mailman/listinfo/openib-general
 
 To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
 


___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-01-26 Thread Steve Wise 
Michael/Vlad:

I've pushed these up to my git tree.  Can you merge them in?  

git://staging.openfabrics.com/~swise/ofed_1_2.git cxgb3

Here is the short log of the commits:

aedc0b3c1681fb550ec4b8d1021caa2ce3dcbfd7 iw_cxgb3: allow doorbell mappings with 
VM_READ set.
5ea83b9e3ec6f9c74040944adb83e4faf6613fe1 Backport Chelsio to rhel5 (2.6.18_FC6).
ff38246f6f07ff25609eaa304a707748904bf2bf Backport sles9sp3: Simulate neigh 
update events by snooping ARP packets
b88d46d10ce15f8ee725454f4998af6497cc13e1 Backport rhel4u4: Simulate neigh 
update events by snooping ARP packets
ab3a817b10da2df2e3d5bf08018be3d0212dc5bd Backport 2.6.11: Simulate neigh update 
events by snooping ARP packets
e545001a94c180c32b8b15d4ca4351506bd50fc2 Backport 2.6.12: Simulate neigh update 
events by snooping ARP packets
8ddafe035c1a997c7625ae1bd42767deed148cb7 Backport 2.6.13: Simulate neigh update 
events by snooping ARP packets
ef260b8242d90edcabdc3153b829eda65d451672 Backport 2.6.14: Simulate neigh update 
events by snooping ARP packets
84c78965a7c6a2a831fb2a49c6936321e2566904 Backport ubuntu606: Simulate neigh 
update events by snooping ARP packets
eb09f52a33471613fc29f898dfad8d9a57238d3e Backport 2.6.15: Simulate neigh update 
events by snooping ARP packets
ddc3ec432bd1898005ab52241d125dd4a71436aa Backport sles10: Simulate neigh update 
events by snooping ARP packets
b4af429744ff06545b2941fc5ef1ab4d6f0c0e77 Backport 2.6.16: Simulate neigh update 
events by snooping ARP packets
fe1a597f3aa409465d5b1b577a3b28c4a002f143 Backport 2.6.17: Simulate neighbour 
update events by snooping ARP packets
9b3bfe5696aa417d38ce903eb345a03d65743dd2 Handle Ethernet neighbour updates 
during route resolution.



___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

2007-01-25 Thread Steve Wise 

Michael/Vlad:

Here are the backports for snooping arp packets to generate neighbour
update netevents.  Also included is the addr.c patch to act on all valid
neigh update events.  If this series looks good to you then I'll push
this up and you all can pull it from my git tree.


Steve.

___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general