Commit c61dd61dec0b79fa22ded8b5caf2e817dc506c24

Authored by Ying Xue
Committed by David S. Miller
1 parent 135daee6d3

tipc: remove 'links' list from tipc_bearer struct

In our ongoing effort to simplify the TIPC locking structure,
we see a need to remove the linked list for tipc_links
in the bearer. This can be explained as follows.

Currently, we have three different ways to access a link,
via three different lists/tables:

1: Via a node hash table:
   Used by the time-critical outgoing/incoming data paths.
   (e.g. link_send_sections_fast() and tipc_recv_msg() ):

grab net_lock(read)
   find node from node hash table
   grab node_lock
       select link
       grab bearer_lock
          send_msg()
       release bearer_lock
   release node lock
release net_lock

2: Via a global linked list for nodes:
   Used by configuration commands (link_cmd_set_value())

grab net_lock(read)
   find node and link from global node list (using link name)
   grab node_lock
       update link
   release node lock
release net_lock

(Same locking order as above. No problem.)

3: Via the bearer's linked link list:
   Used by notifications from interface (e.g. tipc_disable_bearer() )

grab net_lock(write)
   grab bearer_lock
      get link ptr from bearer's link list
      get node from link
      grab node_lock
         delete link
      release node lock
   release bearer_lock
release net_lock

(Different order from above, but works because we grab the
outer net_lock in write mode first, excluding all other access.)

The first major goal in our simplification effort is to get rid
of the "big" net_lock, replacing it with rcu-locks when accessing
the node list and node hash array. This will come in a later patch
series.

But to get there we first need to rewrite access methods ##2 and 3,
since removal of net_lock would introduce three major problems:

a) In access method #2, we access the link before taking the
   protecting node_lock. This will not work once net_lock is gone,
   so we will have to change the access order. We will deal with
   this in a later commit in this series, "tipc: add node lock
   protection to link found by link_find_link()".

b) When the outer protection from net_lock is gone, taking
   bearer_lock and node_lock in opposite order of method 1) and 2)
   will become an obvious deadlock hazard. This is fixed in the
   commit ("tipc: remove bearer_lock from tipc_bearer struct")
   later in this series.

c) Similar to what is described in problem a), access method #3
   starts with using a link pointer that is unprotected by node_lock,
   in order to via that pointer find the correct node struct and
   lock it. Before we remove net_lock, this access order must be
   altered. This is what we do with this commit.

We can avoid introducing problem problem c) by even here using the
global node list to find the node, before accessing its links. When
we loop though the node list we use the own bearer identity as search
criteria, thus easily finding the links that are associated to the
resetting/disabling bearer. It should be noted that although this
method is somewhat slower than the current list traversal, it is in
no way time critical. This is only about resetting or deleting links,
something that must be considered relatively infrequent events.

As a bonus, we can get rid of the mutual pointers between links and
bearers. After this commit, pointer dependency go in one direction
only: from the link to the bearer.

This commit pre-empts introduction of problem c) as described above.

Signed-off-by: Ying Xue <ying.xue@windriver.com>
Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

Showing 5 changed files with 30 additions and 54 deletions Side-by-side Diff

... ... @@ -327,7 +327,6 @@
327 327 b_ptr->net_plane = bearer_id + 'A';
328 328 b_ptr->active = 1;
329 329 b_ptr->priority = priority;
330   - INIT_LIST_HEAD(&b_ptr->links);
331 330 spin_lock_init(&b_ptr->lock);
332 331  
333 332 res = tipc_disc_create(b_ptr, &b_ptr->bcast_addr, disc_domain);
... ... @@ -353,7 +352,7 @@
353 352 read_lock_bh(&tipc_net_lock);
354 353 pr_info("Resetting bearer <%s>\n", b_ptr->name);
355 354 spin_lock_bh(&b_ptr->lock);
356   - tipc_link_reset_list(b_ptr);
  355 + tipc_link_reset_list(b_ptr->identity);
357 356 spin_unlock_bh(&b_ptr->lock);
358 357 read_unlock_bh(&tipc_net_lock);
359 358 return 0;
... ... @@ -371,7 +370,7 @@
371 370 pr_info("Disabling bearer <%s>\n", b_ptr->name);
372 371 spin_lock_bh(&b_ptr->lock);
373 372 b_ptr->media->disable_media(b_ptr);
374   - tipc_link_delete_list(b_ptr);
  373 + tipc_link_delete_list(b_ptr->identity);
375 374 temp_req = b_ptr->link_req;
376 375 b_ptr->link_req = NULL;
377 376 spin_unlock_bh(&b_ptr->lock);
... ... @@ -120,7 +120,6 @@
120 120 * @tolerance: default link tolerance for bearer
121 121 * @identity: array index of this bearer within TIPC bearer array
122 122 * @link_req: ptr to (optional) structure making periodic link setup requests
123   - * @links: list of non-congested links associated with bearer
124 123 * @active: non-zero if bearer structure is represents a bearer
125 124 * @net_plane: network plane ('A' through 'H') currently associated with bearer
126 125 * @nodes: indicates which nodes in cluster can be reached through bearer
... ... @@ -142,7 +141,6 @@
142 141 u32 tolerance;
143 142 u32 identity;
144 143 struct tipc_link_req *link_req;
145   - struct list_head links;
146 144 int active;
147 145 char net_plane;
148 146 struct tipc_node_map nodes;
1 1 /*
2 2 * net/tipc/core.c: TIPC module code
3 3 *
4   - * Copyright (c) 2003-2006, Ericsson AB
  4 + * Copyright (c) 2003-2006, 2013, Ericsson AB
5 5 * Copyright (c) 2005-2006, 2010-2013, Wind River Systems
6 6 * All rights reserved.
7 7 *
... ... @@ -147,11 +147,6 @@
147 147 /**
148 148 * link_timeout - handle expiration of link timer
149 149 * @l_ptr: pointer to link
150   - *
151   - * This routine must not grab "tipc_net_lock" to avoid a potential deadlock conflict
152   - * with tipc_link_delete(). (There is no risk that the node will be deleted by
153   - * another thread because tipc_link_delete() always cancels the link timer before
154   - * tipc_node_delete() is called.)
155 150 */
156 151 static void link_timeout(struct tipc_link *l_ptr)
157 152 {
... ... @@ -213,8 +208,8 @@
213 208 * Returns pointer to link.
214 209 */
215 210 struct tipc_link *tipc_link_create(struct tipc_node *n_ptr,
216   - struct tipc_bearer *b_ptr,
217   - const struct tipc_media_addr *media_addr)
  211 + struct tipc_bearer *b_ptr,
  212 + const struct tipc_media_addr *media_addr)
218 213 {
219 214 struct tipc_link *l_ptr;
220 215 struct tipc_msg *msg;
221 216  
222 217  
223 218  
224 219  
... ... @@ -279,47 +274,32 @@
279 274  
280 275 k_init_timer(&l_ptr->timer, (Handler)link_timeout,
281 276 (unsigned long)l_ptr);
282   - list_add_tail(&l_ptr->link_list, &b_ptr->links);
283 277  
284 278 link_state_event(l_ptr, STARTING_EVT);
285 279  
286 280 return l_ptr;
287 281 }
288 282  
289   -/**
290   - * tipc_link_delete - delete a link
291   - * @l_ptr: pointer to link
292   - *
293   - * Note: 'tipc_net_lock' is write_locked, bearer is locked.
294   - * This routine must not grab the node lock until after link timer cancellation
295   - * to avoid a potential deadlock situation.
296   - */
297   -void tipc_link_delete(struct tipc_link *l_ptr)
298   -{
299   - if (!l_ptr) {
300   - pr_err("Attempt to delete non-existent link\n");
301   - return;
302   - }
303 283  
304   - k_cancel_timer(&l_ptr->timer);
305   -
306   - tipc_node_lock(l_ptr->owner);
307   - tipc_link_reset(l_ptr);
308   - tipc_node_detach_link(l_ptr->owner, l_ptr);
309   - tipc_link_purge_queues(l_ptr);
310   - list_del_init(&l_ptr->link_list);
311   - tipc_node_unlock(l_ptr->owner);
312   - k_term_timer(&l_ptr->timer);
313   - kfree(l_ptr);
314   -}
315   -
316   -void tipc_link_delete_list(struct tipc_bearer *b_ptr)
  284 +void tipc_link_delete_list(unsigned int bearer_id)
317 285 {
318 286 struct tipc_link *l_ptr;
319   - struct tipc_link *temp_l_ptr;
  287 + struct tipc_node *n_ptr;
320 288  
321   - list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
322   - tipc_link_delete(l_ptr);
  289 + list_for_each_entry(n_ptr, &tipc_node_list, list) {
  290 + spin_lock_bh(&n_ptr->lock);
  291 + l_ptr = n_ptr->links[bearer_id];
  292 + if (l_ptr) {
  293 + tipc_link_reset(l_ptr);
  294 + tipc_node_detach_link(n_ptr, l_ptr);
  295 + spin_unlock_bh(&n_ptr->lock);
  296 +
  297 + /* Nobody else can access this link now: */
  298 + del_timer_sync(&l_ptr->timer);
  299 + kfree(l_ptr);
  300 + continue;
  301 + }
  302 + spin_unlock_bh(&n_ptr->lock);
323 303 }
324 304 }
325 305  
326 306  
327 307  
328 308  
... ... @@ -470,15 +450,16 @@
470 450 link_reset_statistics(l_ptr);
471 451 }
472 452  
473   -void tipc_link_reset_list(struct tipc_bearer *b_ptr)
  453 +void tipc_link_reset_list(unsigned int bearer_id)
474 454 {
475 455 struct tipc_link *l_ptr;
  456 + struct tipc_node *n_ptr;
476 457  
477   - list_for_each_entry(l_ptr, &b_ptr->links, link_list) {
478   - struct tipc_node *n_ptr = l_ptr->owner;
479   -
  458 + list_for_each_entry(n_ptr, &tipc_node_list, list) {
480 459 spin_lock_bh(&n_ptr->lock);
481   - tipc_link_reset(l_ptr);
  460 + l_ptr = n_ptr->links[bearer_id];
  461 + if (l_ptr)
  462 + tipc_link_reset(l_ptr);
482 463 spin_unlock_bh(&n_ptr->lock);
483 464 }
484 465 }
... ... @@ -59,6 +59,7 @@
59 59 /* Link endpoint execution states
60 60 */
61 61 #define LINK_STARTED 0x0001
  62 +#define LINK_STOPPED 0x0002
62 63  
63 64 /* Starting value for maximum packet size negotiation on unicast links
64 65 * (unless bearer MTU is less)
... ... @@ -102,7 +103,6 @@
102 103 * @media_addr: media address to use when sending messages over link
103 104 * @timer: link timer
104 105 * @owner: pointer to peer node
105   - * @link_list: adjacent links in bearer's list of links
106 106 * @flags: execution state flags for link endpoint instance
107 107 * @checkpoint: reference point for triggering link continuity checking
108 108 * @peer_session: link session # being used by peer end of link
... ... @@ -149,7 +149,6 @@
149 149 struct tipc_media_addr media_addr;
150 150 struct timer_list timer;
151 151 struct tipc_node *owner;
152   - struct list_head link_list;
153 152  
154 153 /* Management and link supervision data */
155 154 unsigned int flags;
156 155  
... ... @@ -215,11 +214,10 @@
215 214 struct tipc_link *tipc_link_create(struct tipc_node *n_ptr,
216 215 struct tipc_bearer *b_ptr,
217 216 const struct tipc_media_addr *media_addr);
218   -void tipc_link_delete(struct tipc_link *l_ptr);
  217 +void tipc_link_delete_list(unsigned int bearer_id);
219 218 void tipc_link_failover_send_queue(struct tipc_link *l_ptr);
220 219 void tipc_link_dup_send_queue(struct tipc_link *l_ptr,
221 220 struct tipc_link *dest);
222   -void tipc_link_delete_list(struct tipc_bearer *b_ptr);
223 221 void tipc_link_reset_fragments(struct tipc_link *l_ptr);
224 222 int tipc_link_is_up(struct tipc_link *l_ptr);
225 223 int tipc_link_is_active(struct tipc_link *l_ptr);
... ... @@ -232,7 +230,7 @@
232 230 struct sk_buff *tipc_link_cmd_reset_stats(const void *req_tlv_area,
233 231 int req_tlv_space);
234 232 void tipc_link_reset(struct tipc_link *l_ptr);
235   -void tipc_link_reset_list(struct tipc_bearer *b_ptr);
  233 +void tipc_link_reset_list(unsigned int bearer_id);
236 234 int tipc_link_send(struct sk_buff *buf, u32 dest, u32 selector);
237 235 void tipc_link_send_names(struct list_head *message_list, u32 dest);
238 236 int tipc_link_send_buf(struct tipc_link *l_ptr, struct sk_buff *buf);