RE: [PATCH 7/32] [TIPC] Multicast link failure now resets all links to nacking node.
On Thu, 22 Jun 2006, Stephens, Allan wrote: [...] Per, I'll leave it to you to decide if you want to address James's concern. But be aware that the link_reset_all() routine is only called to handle emergency situations when TIPC's multicast link has run into serious problems and is trying to recover. Most systems will never follow this path, so the cost of the unnecessary local_bh_disable()/local_bh_enable() pairing shouldn't have any real impact on the overall performance of TIPC. I tend to agree with you Allan. Since local_bh_disable()/local_bh_enable() supports nesting and this code is only called in extreme situations I don't think it's a real problem. /Per - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/32] [TIPC] Multicast link failure now resets all links to nacking node.
From: Allan Stephens [EMAIL PROTECTED]
This fix prevents node from crashing.
Signed-off-by: Allan Stephens [EMAIL PROTECTED]
Signed-off-by: Per Liden [EMAIL PROTECTED]
---
net/tipc/bcast.c | 32 +++---
net/tipc/link.c | 124 +-
2 files changed, 128 insertions(+), 28 deletions(-)
diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 2c4ecbe..00691b7 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -49,13 +49,19 @@ #include bearer.h
#include name_table.h
#include bcast.h
-
#define MAX_PKT_DEFAULT_MCAST 1500 /* bcast link max packet size (fixed) */
#define BCLINK_WIN_DEFAULT 20 /* bcast link window size (default) */
#define BCLINK_LOG_BUF_SIZE 0
+/*
+ * Loss rate for incoming broadcast frames; used to test retransmission code.
+ * Set to N to cause every N'th frame to be discarded; 0 = don't discard any.
+ */
+
+#define TIPC_BCAST_LOSS_RATE 0
+
/**
* struct bcbearer_pair - a pair of bearers used by broadcast link
* @primary: pointer to primary bearer
@@ -165,21 +171,18 @@ static int bclink_ack_allowed(u32 n)
* @after: sequence number of last packet to *not* retransmit
* @to: sequence number of last packet to retransmit
*
- * Called with 'node' locked, bc_lock unlocked
+ * Called with bc_lock locked
*/
static void bclink_retransmit_pkt(u32 after, u32 to)
{
struct sk_buff *buf;
- spin_lock_bh(bc_lock);
buf = bcl-first_out;
while (buf less_eq(buf_seqno(buf), after)) {
buf = buf-next;
}
- if (buf != NULL)
- tipc_link_retransmit(bcl, buf, mod(to - after));
- spin_unlock_bh(bc_lock);
+ tipc_link_retransmit(bcl, buf, mod(to - after));
}
/**
@@ -399,7 +402,10 @@ int tipc_bclink_send_msg(struct sk_buff
*/
void tipc_bclink_recv_pkt(struct sk_buff *buf)
-{
+{
+#if (TIPC_BCAST_LOSS_RATE)
+ static int rx_count = 0;
+#endif
struct tipc_msg *msg = buf_msg(buf);
struct node* node = tipc_node_find(msg_prevnode(msg));
u32 next_in;
@@ -420,9 +426,13 @@ void tipc_bclink_recv_pkt(struct sk_buff
tipc_node_lock(node);
tipc_bclink_acknowledge(node, msg_bcast_ack(msg));
tipc_node_unlock(node);
+ spin_lock_bh(bc_lock);
bcl-stats.recv_nacks++;
+ bcl-owner-next = node; /* remember requestor */
bclink_retransmit_pkt(msg_bcgap_after(msg),
msg_bcgap_to(msg));
+ bcl-owner-next = NULL;
+ spin_unlock_bh(bc_lock);
} else {
tipc_bclink_peek_nack(msg_destnode(msg),
msg_bcast_tag(msg),
@@ -433,6 +443,14 @@ void tipc_bclink_recv_pkt(struct sk_buff
return;
}
+#if (TIPC_BCAST_LOSS_RATE)
+ if (++rx_count == TIPC_BCAST_LOSS_RATE) {
+ rx_count = 0;
+ buf_discard(buf);
+ return;
+ }
+#endif
+
tipc_node_lock(node);
receive:
deferred = node-bclink.deferred_head;
diff --git a/net/tipc/link.c b/net/tipc/link.c
index 955b87d..ba7d3f1 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1604,40 +1604,121 @@ void tipc_link_push_queue(struct link *l
tipc_bearer_schedule(l_ptr-b_ptr, l_ptr);
}
+static void link_reset_all(unsigned long addr)
+{
+ struct node *n_ptr;
+ char addr_string[16];
+ u32 i;
+
+ read_lock_bh(tipc_net_lock);
+ n_ptr = tipc_node_find((u32)addr);
+ if (!n_ptr) {
+ read_unlock_bh(tipc_net_lock);
+ return; /* node no longer exists */
+ }
+
+ tipc_node_lock(n_ptr);
+
+ warn(Resetting all links to %s\n,
+addr_string_fill(addr_string, n_ptr-addr));
+
+ for (i = 0; i MAX_BEARERS; i++) {
+ if (n_ptr-links[i]) {
+ link_print(n_ptr-links[i], TIPC_OUTPUT,
+ Resetting link\n);
+ tipc_link_reset(n_ptr-links[i]);
+ }
+ }
+
+ tipc_node_unlock(n_ptr);
+ read_unlock_bh(tipc_net_lock);
+}
+
+static void link_retransmit_failure(struct link *l_ptr, struct sk_buff *buf)
+{
+ struct tipc_msg *msg = buf_msg(buf);
+
+ warn(Retransmission failure on link %s\n, l_ptr-name);
+ tipc_msg_print(TIPC_OUTPUT, msg, RETR-FAIL);
+
+ if (l_ptr-addr) {
+
+ /* Handle failure on standard link */
+
+ link_print(l_ptr, TIPC_OUTPUT, Resetting link\n);
+ tipc_link_reset(l_ptr);
+
+ } else {
+
+ /* Handle failure on broadcast link */
+
+ struct node *n_ptr;
+ char addr_string[16];
+
+
Re: [PATCH 7/32] [TIPC] Multicast link failure now resets all links to nacking node.
On Thu, 22 Jun 2006, Per Liden wrote:
+static void link_reset_all(unsigned long addr)
+{
+ struct node *n_ptr;
+ char addr_string[16];
+ u32 i;
+
+ read_lock_bh(tipc_net_lock);
+ n_ptr = tipc_node_find((u32)addr);
+ if (!n_ptr) {
+ read_unlock_bh(tipc_net_lock);
+ return; /* node no longer exists */
+ }
+
+ tipc_node_lock(n_ptr);
You already have bh's disabled here, and tipc_node_lock() also disables
them.
Not sure if it's really worth worrying about but if so, you could perhaps
implement tipc_node_lock_bh() and tipc_node_lock().
- James
--
James Morris
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 7/32] [TIPC] Multicast link failure now resets all links to nacking node.
Nice observation, James. As a relative newcomer to the official Linux
kernel development world, I'm impressed that non-TIPC folks are looking
at TIPC changes closely enough to see things like this!
Per, I'll leave it to you to decide if you want to address James's
concern. But be aware that the link_reset_all() routine is only called
to handle emergency situations when TIPC's multicast link has run into
serious problems and is trying to recover. Most systems will never
follow this path, so the cost of the unnecessary
local_bh_disable()/local_bh_enable() pairing shouldn't have any real
impact on the overall performance of TIPC.
Regards,
Al Stephens
-Original Message-
From: James Morris [mailto:[EMAIL PROTECTED]
Sent: Thursday, June 22, 2006 2:51 PM
To: Per Liden
Cc: David Miller; netdev@vger.kernel.org; Stephens, Allan
Subject: Re: [PATCH 7/32] [TIPC] Multicast link failure now
resets all links to nacking node.
On Thu, 22 Jun 2006, Per Liden wrote:
+static void link_reset_all(unsigned long addr) {
+ struct node *n_ptr;
+ char addr_string[16];
+ u32 i;
+
+ read_lock_bh(tipc_net_lock);
+ n_ptr = tipc_node_find((u32)addr);
+ if (!n_ptr) {
+ read_unlock_bh(tipc_net_lock);
+ return; /* node no longer exists */
+ }
+
+ tipc_node_lock(n_ptr);
You already have bh's disabled here, and tipc_node_lock()
also disables them.
Not sure if it's really worth worrying about but if so, you
could perhaps implement tipc_node_lock_bh() and tipc_node_lock().
- James
--
James Morris
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
