Menu
▾
▴
tipc-discussion
|
From: Ying X. <yin...@wi...> - 2013-08-08 08:26:47
|
v2 Changes:
- revise changelogs of all relevant patches.
- drop the patch "[PATCH net-next v1 06/30] tipc: avoid to launch
different thread to call link_state_event". As Jason said, it's
meaningful to let tipc_link_start() run in tasklet context.
- merge patch "PATCH net-next v1 01/30] tipc: remove bearer setup
works" into patch #4 (move registration of TIPC packet handler to
media start).
- divide patch "PATCH net-next v1 10/30] tipc: convert static media
array to hash list" into two patches like patch #2 and patch #3.
- merge patch #13 "[PATCH net-next v1 13/30] tipc: move functions of
registering media into tipc_core_start routine", patch #14 "[PATCH
net-next v1 14/30] tipc: eliminate tipc core net start/stop routines"
and patch #15 "[PATCH net-next v1 15/30] tipc: use config_mutex to
protect tipc_net_stop routine" to one patch #6.
- divide patch #16 into two patches: patch #7 and patch #8.
- add a new patch #9.
- now the mapping relationship between v1 and v2 will be:
1. old patch #1 is to new patch #4;
2. old patch #2, #3 and #4 are included into the small
non-controversial series;
3. old patch #4 is dropped in the series.
4. old patch #9 is mapped to patch #1.
5. old patch #10 is divided into patch #2 and #3.
6. old patch #11 is mapped to patch #4.
7. old patch #12 is mapped to patch #5.
8. old patch #13, #14 and #15 is patch #6.
9. old patch #16 is divided into patch #7 and #8.
10. old patch #17 is mapped to patch #10.
11. the rest of patches in v1 will be sent out recently.
Ying Xue (10):
tipc: involve tipc_unregister_media routine
tipc: make bearer variable in media structure synchronous
tipc: get rid of af_packet_priv usage when register TIPC protocol
handler
tipc: move registration of TIPC packet handler to media start
tipc: remove flush_scheduled_work() from media stop routines
tipc: expand config_muex usage
tipc: convert tipc_bearers array to pointer array
tipc: make media structure contain tipc_bearer object
tipc: add common unwind section to reduce duplicated code
tipc: use bearer lock to protect links list when link is created
net/tipc/bcast.c | 6 +-
net/tipc/bearer.c | 81 ++++++++++++++------
net/tipc/bearer.h | 7 +-
net/tipc/config.c | 4 +-
net/tipc/config.h | 4 +-
net/tipc/core.c | 37 ++-------
net/tipc/core.h | 1 -
net/tipc/discover.c | 17 ++---
net/tipc/eth_media.c | 203 +++++++++++++++++++++++++-------------------------
net/tipc/ib_media.c | 189 ++++++++++++++++++++++++----------------------
net/tipc/link.c | 2 -
net/tipc/net.c | 3 +
12 files changed, 286 insertions(+), 268 deletions(-)
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:38:43
|
v3 changes:
- add patch #1 and patch #7
- rebase v2 on the series of "[PATCH net-next v2 0/6] tipc: some small
patches" sbumitted by Ying at 9/29 and the patch of "[PATCH net-next
1/1] tipc: simplify the link lookup routine" sent by Jon at 9/6.
v2 changes:
- revise changelogs of all relevant patches.
- drop the patch "[PATCH net-next v1 06/30] tipc: avoid to launch
different thread to call link_state_event". As Jason said, it's
meaningful to let tipc_link_start() run in tasklet context.
- merge patch "PATCH net-next v1 01/30] tipc: remove bearer setup
works" into patch #4 (move registration of TIPC packet handler to
media start).
- divide patch "PATCH net-next v1 10/30] tipc: convert static media
array to hash list" into two patches like patch #2 and patch #3.
- merge patch #13 "[PATCH net-next v1 13/30] tipc: move functions of
registering media into tipc_core_start routine", patch #14 "[PATCH
net-next v1 14/30] tipc: eliminate tipc core net start/stop routines"
and patch #15 "[PATCH net-next v1 15/30] tipc: use config_mutex to
protect tipc_net_stop routine" to one patch #6.
- divide patch #16 into two patches: patch #7 and patch #8.
- add a new patch #9.
- now the mapping relationship between v1 and v2 will be:
1. old patch #1 is to new patch #4;
2. old patch #2, #3 and #4 are included into the small
non-controversial series;
3. old patch #4 is dropped in the series.
4. old patch #9 is mapped to patch #1.
5. old patch #10 is divided into patch #2 and #3.
6. old patch #11 is mapped to patch #4.
7. old patch #12 is mapped to patch #5.
8. old patch #13, #14 and #15 is patch #6.
9. old patch #16 is divided into patch #7 and #8.
10. old patch #17 is mapped to patch #10.
11. the rest of patches in v1 will be sent out recently.
Ying Xue (12):
tipc: convert 'blocked' flag in struct tipc_bearer to atomic_t
tipc: involve tipc_unregister_media routine
tipc: make bearer variable in media structure synchronous
tipc: get rid of af_packet_priv usage when register TIPC protocol
handler
tipc: move registration of TIPC packet handler to media start
tipc: remove flush_scheduled_work() from media stop routines
tipc: remove extern keywords from function declarations
tipc: expand config_muex usage
tipc: convert tipc_bearers array to pointer array
tipc: make media structure contain tipc_bearer object
tipc: add common unwind section to reduce duplicated code
tipc: use bearer lock to protect links list when link is created
net/tipc/bcast.c | 10 +--
net/tipc/bearer.c | 111 +++++++++++++++------------
net/tipc/bearer.h | 12 +--
net/tipc/config.c | 4 +-
net/tipc/config.h | 4 +-
net/tipc/core.c | 37 ++-------
net/tipc/core.h | 27 ++++---
net/tipc/discover.c | 22 +++---
net/tipc/eth_media.c | 206 +++++++++++++++++++++++++-------------------------
net/tipc/ib_media.c | 192 +++++++++++++++++++++++-----------------------
net/tipc/link.c | 14 ++--
net/tipc/net.c | 3 +
12 files changed, 315 insertions(+), 327 deletions(-)
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:38:44
|
The 'blocked' flag in struct tipc_bearer is currently protected by
a big spinlock aggregated into tipc_bearer. We want to reduce all
dependencies to this lock, as part of our effort to make the locking
policy simpler and more manageable.
Furthermore, the functions tipc_disc_recv_msg() and disc_send_msg()
access the flag without taking this lock at all, implying a risk that
they may occasionally obtain a wrong value and drop sending/receiving
of discovery messages. Since discovery messages are timer driven and
best-effort, the latter problem is not fatal, but it is clearly avoidable.
By converting the type of the flag from int to atomic_t we eliminate
both the spinlock dependency and the current race condition.
We also make two small code changes:
1) The functions tipc_continue() and bearer_blocked() now become
one-liners doing a single access to the flag. So we just remove
those redundant functions.
2) The initialization of the flag is moved from the enable_media()
functions in ib_media.c and eth_media.c respecively, to the generic
tipc_enable_bearer() function, so that it is done in one place only.
Signed-off-by: Ying Xue <yin...@wi...>
---
net/tipc/bcast.c | 4 ++--
net/tipc/bearer.c | 33 ++++-----------------------------
net/tipc/bearer.h | 5 +----
net/tipc/discover.c | 5 +++--
net/tipc/eth_media.c | 9 ++++-----
net/tipc/ib_media.c | 9 ++++-----
net/tipc/link.c | 12 ++++++------
7 files changed, 24 insertions(+), 53 deletions(-)
diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index 716de1a..c78f30c 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -615,8 +615,8 @@ static int tipc_bcbearer_send(struct sk_buff *buf, struct tipc_bearer *unused1,
if (!p)
break; /* No more bearers to try */
- if (tipc_bearer_blocked(p)) {
- if (!s || tipc_bearer_blocked(s))
+ if (atomic_read(&p->blocked)) {
+ if (!s || atomic_read(&s->blocked))
continue; /* Can't use either bearer */
b = s;
}
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 3f9707a..17f9824 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -275,31 +275,6 @@ void tipc_bearer_remove_dest(struct tipc_bearer *b_ptr, u32 dest)
tipc_disc_remove_dest(b_ptr->link_req);
}
-/*
- * Interrupt enabling new requests after bearer blocking:
- * See bearer_send().
- */
-void tipc_continue(struct tipc_bearer *b)
-{
- spin_lock_bh(&b->lock);
- b->blocked = 0;
- spin_unlock_bh(&b->lock);
-}
-
-/*
- * tipc_bearer_blocked - determines if bearer is currently blocked
- */
-int tipc_bearer_blocked(struct tipc_bearer *b)
-{
- int res;
-
- spin_lock_bh(&b->lock);
- res = b->blocked;
- spin_unlock_bh(&b->lock);
-
- return res;
-}
-
/**
* tipc_enable_bearer - enable bearer with the given name
*/
@@ -387,6 +362,8 @@ restart:
b_ptr = &tipc_bearers[bearer_id];
strcpy(b_ptr->name, name);
+ atomic_set(&b_ptr->blocked, 0);
+ smp_mb();
res = m_ptr->enable_media(b_ptr);
if (res) {
pr_warn("Bearer <%s> rejected, enable failure (%d)\n",
@@ -429,8 +406,8 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
read_lock_bh(&tipc_net_lock);
pr_info("Blocking bearer <%s>\n", b_ptr->name);
+ atomic_add_unless(&b_ptr->blocked, 1, 1);
spin_lock_bh(&b_ptr->lock);
- b_ptr->blocked = 1;
list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
struct tipc_node *n_ptr = l_ptr->owner;
@@ -455,8 +432,8 @@ static void bearer_disable(struct tipc_bearer *b_ptr)
struct tipc_link_req *temp_req;
pr_info("Disabling bearer <%s>\n", b_ptr->name);
+ atomic_add_unless(&b_ptr->blocked, 1, 1);
spin_lock_bh(&b_ptr->lock);
- b_ptr->blocked = 1;
b_ptr->media->disable_media(b_ptr);
list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
tipc_link_delete(l_ptr);
@@ -489,8 +466,6 @@ int tipc_disable_bearer(const char *name)
return res;
}
-
-
void tipc_bearer_stop(void)
{
u32 i;
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index e5e04be..8695d4a 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -130,7 +130,7 @@ struct tipc_media {
struct tipc_bearer {
void *usr_handle; /* initalized by media */
u32 mtu; /* initalized by media */
- int blocked; /* initalized by media */
+ atomic_t blocked;
struct tipc_media_addr addr; /* initalized by media */
char name[TIPC_MAX_BEARER_NAME];
spinlock_t lock;
@@ -164,8 +164,6 @@ int tipc_register_media(struct tipc_media *m_ptr);
void tipc_recv_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr);
int tipc_block_bearer(struct tipc_bearer *b_ptr);
-void tipc_continue(struct tipc_bearer *tb_ptr);
-
int tipc_enable_bearer(const char *bearer_name, u32 disc_domain, u32 priority);
int tipc_disable_bearer(const char *name);
@@ -194,7 +192,6 @@ void tipc_bearer_remove_dest(struct tipc_bearer *b_ptr, u32 dest);
struct tipc_bearer *tipc_bearer_find(const char *name);
struct tipc_bearer *tipc_bearer_find_interface(const char *if_name);
struct tipc_media *tipc_media_find(const char *name);
-int tipc_bearer_blocked(struct tipc_bearer *b_ptr);
void tipc_bearer_stop(void);
/**
diff --git a/net/tipc/discover.c b/net/tipc/discover.c
index ecc758c..d8b49fb 100644
--- a/net/tipc/discover.c
+++ b/net/tipc/discover.c
@@ -239,7 +239,8 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr)
/* Accept discovery message & send response, if necessary */
link_fully_up = link_working_working(link);
- if ((type == DSC_REQ_MSG) && !link_fully_up && !b_ptr->blocked) {
+ if ((type == DSC_REQ_MSG) && !link_fully_up &&
+ !atomic_read(&b_ptr->blocked)) {
rbuf = tipc_disc_init_msg(DSC_RESP_MSG, orig, b_ptr);
if (rbuf) {
tipc_bearer_send(b_ptr, rbuf, &media_addr);
@@ -293,7 +294,7 @@ void tipc_disc_remove_dest(struct tipc_link_req *req)
*/
static void disc_send_msg(struct tipc_link_req *req)
{
- if (!req->bearer->blocked)
+ if (!atomic_read(&req->bearer->blocked))
tipc_bearer_send(req->bearer, req->buf, &req->dest);
}
diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index f80d59f..32d01e8 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -1,7 +1,7 @@
/*
* net/tipc/eth_media.c: Ethernet bearer support for TIPC
*
- * Copyright (c) 2001-2007, Ericsson AB
+ * Copyright (c) 2001-2007, 2013, Ericsson AB
* Copyright (c) 2005-2008, 2011-2013, Wind River Systems
* All rights reserved.
*
@@ -199,7 +199,6 @@ static int enable_media(struct tipc_bearer *tb_ptr)
tb_ptr->bcast_addr.media_id = TIPC_MEDIA_TYPE_ETH;
tb_ptr->bcast_addr.broadcast = 1;
tb_ptr->mtu = dev->mtu;
- tb_ptr->blocked = 0;
eth_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr);
return 0;
}
@@ -263,12 +262,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
switch (evt) {
case NETDEV_CHANGE:
if (netif_carrier_ok(dev))
- tipc_continue(eb_ptr->bearer);
+ atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
else
tipc_block_bearer(eb_ptr->bearer);
break;
case NETDEV_UP:
- tipc_continue(eb_ptr->bearer);
+ atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
break;
case NETDEV_DOWN:
tipc_block_bearer(eb_ptr->bearer);
@@ -276,7 +275,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
case NETDEV_CHANGEMTU:
case NETDEV_CHANGEADDR:
tipc_block_bearer(eb_ptr->bearer);
- tipc_continue(eb_ptr->bearer);
+ atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
break;
case NETDEV_UNREGISTER:
case NETDEV_CHANGENAME:
diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
index c139892..5241a5b 100644
--- a/net/tipc/ib_media.c
+++ b/net/tipc/ib_media.c
@@ -5,7 +5,7 @@
*
* Based on eth_media.c, which carries the following copyright notice:
*
- * Copyright (c) 2001-2007, Ericsson AB
+ * Copyright (c) 2001-2007, 2013, Ericsson AB
* Copyright (c) 2005-2008, 2011, Wind River Systems
* All rights reserved.
*
@@ -192,7 +192,6 @@ static int enable_media(struct tipc_bearer *tb_ptr)
tb_ptr->bcast_addr.media_id = TIPC_MEDIA_TYPE_IB;
tb_ptr->bcast_addr.broadcast = 1;
tb_ptr->mtu = dev->mtu;
- tb_ptr->blocked = 0;
ib_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr);
return 0;
}
@@ -256,12 +255,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
switch (evt) {
case NETDEV_CHANGE:
if (netif_carrier_ok(dev))
- tipc_continue(ib_ptr->bearer);
+ atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
else
tipc_block_bearer(ib_ptr->bearer);
break;
case NETDEV_UP:
- tipc_continue(ib_ptr->bearer);
+ atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
break;
case NETDEV_DOWN:
tipc_block_bearer(ib_ptr->bearer);
@@ -269,7 +268,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
case NETDEV_CHANGEMTU:
case NETDEV_CHANGEADDR:
tipc_block_bearer(ib_ptr->bearer);
- tipc_continue(ib_ptr->bearer);
+ atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
break;
case NETDEV_UNREGISTER:
case NETDEV_CHANGENAME:
diff --git a/net/tipc/link.c b/net/tipc/link.c
index 4d1297e..8c0e9a3 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -796,7 +796,7 @@ int tipc_link_send_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
return link_send_long_buf(l_ptr, buf);
/* Packet can be queued or sent. */
- if (likely(!tipc_bearer_blocked(l_ptr->b_ptr) &&
+ if (likely(!atomic_read(&l_ptr->b_ptr->blocked) &&
!link_congested(l_ptr))) {
link_add_to_outqueue(l_ptr, buf, msg);
@@ -963,7 +963,7 @@ static int link_send_buf_fast(struct tipc_link *l_ptr, struct sk_buff *buf,
if (likely(!link_congested(l_ptr))) {
if (likely(msg_size(msg) <= l_ptr->max_pkt)) {
- if (likely(!tipc_bearer_blocked(l_ptr->b_ptr))) {
+ if (likely(!atomic_read(&l_ptr->b_ptr->blocked))) {
link_add_to_outqueue(l_ptr, buf, msg);
tipc_bearer_send(l_ptr->b_ptr, buf,
&l_ptr->media_addr);
@@ -1020,7 +1020,7 @@ exit:
/* Exit if link (or bearer) is congested */
if (link_congested(l_ptr) ||
- tipc_bearer_blocked(l_ptr->b_ptr)) {
+ atomic_read(&l_ptr->b_ptr->blocked)) {
res = link_schedule_port(l_ptr,
sender->ref, res);
goto exit;
@@ -1287,7 +1287,7 @@ void tipc_link_push_queue(struct tipc_link *l_ptr)
{
u32 res;
- if (tipc_bearer_blocked(l_ptr->b_ptr))
+ if (atomic_read(&l_ptr->b_ptr->blocked))
return;
do {
@@ -1376,7 +1376,7 @@ void tipc_link_retransmit(struct tipc_link *l_ptr, struct sk_buff *buf,
msg = buf_msg(buf);
- if (tipc_bearer_blocked(l_ptr->b_ptr)) {
+ if (atomic_read(&l_ptr->b_ptr->blocked)) {
if (l_ptr->retransm_queue_size == 0) {
l_ptr->retransm_queue_head = msg_seqno(msg);
l_ptr->retransm_queue_size = retransmits;
@@ -1870,7 +1870,7 @@ void tipc_link_send_proto_msg(struct tipc_link *l_ptr, u32 msg_typ,
buf->priority = TC_PRIO_CONTROL;
/* Defer message if bearer is already blocked */
- if (tipc_bearer_blocked(l_ptr->b_ptr)) {
+ if (atomic_read(&l_ptr->b_ptr->blocked)) {
l_ptr->proto_msg_queue = buf;
return;
}
--
1.7.9.5
|
|
From: Jon M. <ma...@do...> - 2013-10-20 21:28:46
|
On 09/30/2013 10:38 AM, Ying Xue wrote:
> The 'blocked' flag in struct tipc_bearer is currently protected by
> a big spinlock aggregated into tipc_bearer. We want to reduce all
> dependencies to this lock, as part of our effort to make the locking
> policy simpler and more manageable.
>
> Furthermore, the functions tipc_disc_recv_msg() and disc_send_msg()
> access the flag without taking this lock at all, implying a risk that
> they may occasionally obtain a wrong value and drop sending/receiving
> of discovery messages. Since discovery messages are timer driven and
> best-effort, the latter problem is not fatal, but it is clearly avoidable.
>
> By converting the type of the flag from int to atomic_t we eliminate
> both the spinlock dependency and the current race condition.
>
> We also make two small code changes:
>
> 1) The functions tipc_continue() and bearer_blocked() now become
> one-liners doing a single access to the flag. So we just remove
> those redundant functions.
>
> 2) The initialization of the flag is moved from the enable_media()
> functions in ib_media.c and eth_media.c respecively, to the generic
> tipc_enable_bearer() function, so that it is done in one place only.
Maybe it is just me, but I still fail to see the benefit (or harm) of this change.
As I understand it, the time line for the current implementation, and the
one you are suggesting, look like this:
Without atomc_t:
------------------------
1: Device driver detects loss of carrier or similar.
2: It issues a call (work_queue?) to all registered users.
3: Until the call has reached the users, they (including TIPC) will continue
to send packets, although the driver can only drop them.
4: The field bearer->blocked is set as a one single write both on 32-bits and
64-bist cPUs, since it is one word, and a small constant value (1 or 0) that is written.
5: Other cores happening to work on the same cache line at that moment
will notice nothing; they will continue to send the packet they were working
on, seeing the previous value of the field.
6: The cache line is flushed and reloaded as soon as next packet is sent or received
on these cores. (I don't think a single 64-byte cache line will remain between
two transmissions. There is way to many data fields involved.)
7: The next time the cache line is reloaded it has the correct value of
'blocked', since the cache coherency HW support ensures that
the cache is updated. Next packet is not sent.
With atomic_t:
------------------
1: Device driver detects loss of carrier or similar.
2: It issues a call (work_queue?) to all registered users.
3: Until the call has reached the users, they (including TIPC) will continue
to send packets, although the driver can only drop them.
4: The field bearer->blocked is set as a single, atomic write, which it would be
anyway, just as in the previous case.
5: Other cores happening to read on the same cache line at that moment
will now see the cache line invalidated, so they will have to reread it with the
new, correct value. The 'volatile' keyword ensures that. As I understand
GCC even puts in memory barrier around volatile variables, but I fail to
to see the relevance here, we don make any other changes to the
bearer struct anyway, so ordereing is not an issue.
6: The cache line now has the correct read value. Packet is not sent.
What have we won? That TIPC may have time to send one more
packet per core than it would otherwise, but nothing can stop
TIPC and others from sending a small number of packets during the
interval between carrier loss detection and until the 'blocked' flag is
updated. The difference it makes if we cache line is invalidated immediately
or at next reload is microscopic in this context.
Admittedly, the harm done is not big either. The memory
barriers put in by GCC around the volatile read does not cost much.
But why make such a change if we cannot motivate it?
Maybe my view is too simplistic, but then somebody has to explain
what is wrong with this reasoning.
Basically, I don't think we need to protect this field with neither
spinlocks, memory barriers nor making it atomic.
Regards
///jon
>
> Signed-off-by: Ying Xue <yin...@wi...>
> ---
> net/tipc/bcast.c | 4 ++--
> net/tipc/bearer.c | 33 ++++-----------------------------
> net/tipc/bearer.h | 5 +----
> net/tipc/discover.c | 5 +++--
> net/tipc/eth_media.c | 9 ++++-----
> net/tipc/ib_media.c | 9 ++++-----
> net/tipc/link.c | 12 ++++++------
> 7 files changed, 24 insertions(+), 53 deletions(-)
>
> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> index 716de1a..c78f30c 100644
> --- a/net/tipc/bcast.c
> +++ b/net/tipc/bcast.c
> @@ -615,8 +615,8 @@ static int tipc_bcbearer_send(struct sk_buff *buf, struct tipc_bearer *unused1,
> if (!p)
> break; /* No more bearers to try */
>
> - if (tipc_bearer_blocked(p)) {
> - if (!s || tipc_bearer_blocked(s))
> + if (atomic_read(&p->blocked)) {
> + if (!s || atomic_read(&s->blocked))
> continue; /* Can't use either bearer */
> b = s;
> }
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 3f9707a..17f9824 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -275,31 +275,6 @@ void tipc_bearer_remove_dest(struct tipc_bearer *b_ptr, u32 dest)
> tipc_disc_remove_dest(b_ptr->link_req);
> }
>
> -/*
> - * Interrupt enabling new requests after bearer blocking:
> - * See bearer_send().
> - */
> -void tipc_continue(struct tipc_bearer *b)
> -{
> - spin_lock_bh(&b->lock);
> - b->blocked = 0;
> - spin_unlock_bh(&b->lock);
> -}
> -
> -/*
> - * tipc_bearer_blocked - determines if bearer is currently blocked
> - */
> -int tipc_bearer_blocked(struct tipc_bearer *b)
> -{
> - int res;
> -
> - spin_lock_bh(&b->lock);
> - res = b->blocked;
> - spin_unlock_bh(&b->lock);
> -
> - return res;
> -}
> -
> /**
> * tipc_enable_bearer - enable bearer with the given name
> */
> @@ -387,6 +362,8 @@ restart:
>
> b_ptr = &tipc_bearers[bearer_id];
> strcpy(b_ptr->name, name);
> + atomic_set(&b_ptr->blocked, 0);
> + smp_mb();
> res = m_ptr->enable_media(b_ptr);
> if (res) {
> pr_warn("Bearer <%s> rejected, enable failure (%d)\n",
> @@ -429,8 +406,8 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
>
> read_lock_bh(&tipc_net_lock);
> pr_info("Blocking bearer <%s>\n", b_ptr->name);
> + atomic_add_unless(&b_ptr->blocked, 1, 1);
> spin_lock_bh(&b_ptr->lock);
> - b_ptr->blocked = 1;
> list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
> struct tipc_node *n_ptr = l_ptr->owner;
>
> @@ -455,8 +432,8 @@ static void bearer_disable(struct tipc_bearer *b_ptr)
> struct tipc_link_req *temp_req;
>
> pr_info("Disabling bearer <%s>\n", b_ptr->name);
> + atomic_add_unless(&b_ptr->blocked, 1, 1);
> spin_lock_bh(&b_ptr->lock);
> - b_ptr->blocked = 1;
> b_ptr->media->disable_media(b_ptr);
> list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
> tipc_link_delete(l_ptr);
> @@ -489,8 +466,6 @@ int tipc_disable_bearer(const char *name)
> return res;
> }
>
> -
> -
> void tipc_bearer_stop(void)
> {
> u32 i;
> diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
> index e5e04be..8695d4a 100644
> --- a/net/tipc/bearer.h
> +++ b/net/tipc/bearer.h
> @@ -130,7 +130,7 @@ struct tipc_media {
> struct tipc_bearer {
> void *usr_handle; /* initalized by media */
> u32 mtu; /* initalized by media */
> - int blocked; /* initalized by media */
> + atomic_t blocked;
> struct tipc_media_addr addr; /* initalized by media */
> char name[TIPC_MAX_BEARER_NAME];
> spinlock_t lock;
> @@ -164,8 +164,6 @@ int tipc_register_media(struct tipc_media *m_ptr);
> void tipc_recv_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr);
>
> int tipc_block_bearer(struct tipc_bearer *b_ptr);
> -void tipc_continue(struct tipc_bearer *tb_ptr);
> -
> int tipc_enable_bearer(const char *bearer_name, u32 disc_domain, u32 priority);
> int tipc_disable_bearer(const char *name);
>
> @@ -194,7 +192,6 @@ void tipc_bearer_remove_dest(struct tipc_bearer *b_ptr, u32 dest);
> struct tipc_bearer *tipc_bearer_find(const char *name);
> struct tipc_bearer *tipc_bearer_find_interface(const char *if_name);
> struct tipc_media *tipc_media_find(const char *name);
> -int tipc_bearer_blocked(struct tipc_bearer *b_ptr);
> void tipc_bearer_stop(void);
>
> /**
> diff --git a/net/tipc/discover.c b/net/tipc/discover.c
> index ecc758c..d8b49fb 100644
> --- a/net/tipc/discover.c
> +++ b/net/tipc/discover.c
> @@ -239,7 +239,8 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr)
> /* Accept discovery message & send response, if necessary */
> link_fully_up = link_working_working(link);
>
> - if ((type == DSC_REQ_MSG) && !link_fully_up && !b_ptr->blocked) {
> + if ((type == DSC_REQ_MSG) && !link_fully_up &&
> + !atomic_read(&b_ptr->blocked)) {
> rbuf = tipc_disc_init_msg(DSC_RESP_MSG, orig, b_ptr);
> if (rbuf) {
> tipc_bearer_send(b_ptr, rbuf, &media_addr);
> @@ -293,7 +294,7 @@ void tipc_disc_remove_dest(struct tipc_link_req *req)
> */
> static void disc_send_msg(struct tipc_link_req *req)
> {
> - if (!req->bearer->blocked)
> + if (!atomic_read(&req->bearer->blocked))
> tipc_bearer_send(req->bearer, req->buf, &req->dest);
> }
>
> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
> index f80d59f..32d01e8 100644
> --- a/net/tipc/eth_media.c
> +++ b/net/tipc/eth_media.c
> @@ -1,7 +1,7 @@
> /*
> * net/tipc/eth_media.c: Ethernet bearer support for TIPC
> *
> - * Copyright (c) 2001-2007, Ericsson AB
> + * Copyright (c) 2001-2007, 2013, Ericsson AB
> * Copyright (c) 2005-2008, 2011-2013, Wind River Systems
> * All rights reserved.
> *
> @@ -199,7 +199,6 @@ static int enable_media(struct tipc_bearer *tb_ptr)
> tb_ptr->bcast_addr.media_id = TIPC_MEDIA_TYPE_ETH;
> tb_ptr->bcast_addr.broadcast = 1;
> tb_ptr->mtu = dev->mtu;
> - tb_ptr->blocked = 0;
> eth_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr);
> return 0;
> }
> @@ -263,12 +262,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
> switch (evt) {
> case NETDEV_CHANGE:
> if (netif_carrier_ok(dev))
> - tipc_continue(eb_ptr->bearer);
> + atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
> else
> tipc_block_bearer(eb_ptr->bearer);
> break;
> case NETDEV_UP:
> - tipc_continue(eb_ptr->bearer);
> + atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
> break;
> case NETDEV_DOWN:
> tipc_block_bearer(eb_ptr->bearer);
> @@ -276,7 +275,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
> case NETDEV_CHANGEMTU:
> case NETDEV_CHANGEADDR:
> tipc_block_bearer(eb_ptr->bearer);
> - tipc_continue(eb_ptr->bearer);
> + atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
> break;
> case NETDEV_UNREGISTER:
> case NETDEV_CHANGENAME:
> diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
> index c139892..5241a5b 100644
> --- a/net/tipc/ib_media.c
> +++ b/net/tipc/ib_media.c
> @@ -5,7 +5,7 @@
> *
> * Based on eth_media.c, which carries the following copyright notice:
> *
> - * Copyright (c) 2001-2007, Ericsson AB
> + * Copyright (c) 2001-2007, 2013, Ericsson AB
> * Copyright (c) 2005-2008, 2011, Wind River Systems
> * All rights reserved.
> *
> @@ -192,7 +192,6 @@ static int enable_media(struct tipc_bearer *tb_ptr)
> tb_ptr->bcast_addr.media_id = TIPC_MEDIA_TYPE_IB;
> tb_ptr->bcast_addr.broadcast = 1;
> tb_ptr->mtu = dev->mtu;
> - tb_ptr->blocked = 0;
> ib_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr);
> return 0;
> }
> @@ -256,12 +255,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
> switch (evt) {
> case NETDEV_CHANGE:
> if (netif_carrier_ok(dev))
> - tipc_continue(ib_ptr->bearer);
> + atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
> else
> tipc_block_bearer(ib_ptr->bearer);
> break;
> case NETDEV_UP:
> - tipc_continue(ib_ptr->bearer);
> + atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
> break;
> case NETDEV_DOWN:
> tipc_block_bearer(ib_ptr->bearer);
> @@ -269,7 +268,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
> case NETDEV_CHANGEMTU:
> case NETDEV_CHANGEADDR:
> tipc_block_bearer(ib_ptr->bearer);
> - tipc_continue(ib_ptr->bearer);
> + atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
> break;
> case NETDEV_UNREGISTER:
> case NETDEV_CHANGENAME:
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 4d1297e..8c0e9a3 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -796,7 +796,7 @@ int tipc_link_send_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
> return link_send_long_buf(l_ptr, buf);
>
> /* Packet can be queued or sent. */
> - if (likely(!tipc_bearer_blocked(l_ptr->b_ptr) &&
> + if (likely(!atomic_read(&l_ptr->b_ptr->blocked) &&
> !link_congested(l_ptr))) {
> link_add_to_outqueue(l_ptr, buf, msg);
>
> @@ -963,7 +963,7 @@ static int link_send_buf_fast(struct tipc_link *l_ptr, struct sk_buff *buf,
>
> if (likely(!link_congested(l_ptr))) {
> if (likely(msg_size(msg) <= l_ptr->max_pkt)) {
> - if (likely(!tipc_bearer_blocked(l_ptr->b_ptr))) {
> + if (likely(!atomic_read(&l_ptr->b_ptr->blocked))) {
> link_add_to_outqueue(l_ptr, buf, msg);
> tipc_bearer_send(l_ptr->b_ptr, buf,
> &l_ptr->media_addr);
> @@ -1020,7 +1020,7 @@ exit:
>
> /* Exit if link (or bearer) is congested */
> if (link_congested(l_ptr) ||
> - tipc_bearer_blocked(l_ptr->b_ptr)) {
> + atomic_read(&l_ptr->b_ptr->blocked)) {
> res = link_schedule_port(l_ptr,
> sender->ref, res);
> goto exit;
> @@ -1287,7 +1287,7 @@ void tipc_link_push_queue(struct tipc_link *l_ptr)
> {
> u32 res;
>
> - if (tipc_bearer_blocked(l_ptr->b_ptr))
> + if (atomic_read(&l_ptr->b_ptr->blocked))
> return;
>
> do {
> @@ -1376,7 +1376,7 @@ void tipc_link_retransmit(struct tipc_link *l_ptr, struct sk_buff *buf,
>
> msg = buf_msg(buf);
>
> - if (tipc_bearer_blocked(l_ptr->b_ptr)) {
> + if (atomic_read(&l_ptr->b_ptr->blocked)) {
> if (l_ptr->retransm_queue_size == 0) {
> l_ptr->retransm_queue_head = msg_seqno(msg);
> l_ptr->retransm_queue_size = retransmits;
> @@ -1870,7 +1870,7 @@ void tipc_link_send_proto_msg(struct tipc_link *l_ptr, u32 msg_typ,
> buf->priority = TC_PRIO_CONTROL;
>
> /* Defer message if bearer is already blocked */
> - if (tipc_bearer_blocked(l_ptr->b_ptr)) {
> + if (atomic_read(&l_ptr->b_ptr->blocked)) {
> l_ptr->proto_msg_queue = buf;
> return;
> }
|
|
From: Ying X. <yin...@wi...> - 2013-10-25 02:14:16
|
On 10/21/2013 05:14 AM, Jon Maloy wrote:
> On 09/30/2013 10:38 AM, Ying Xue wrote:
>> The 'blocked' flag in struct tipc_bearer is currently protected by
>> a big spinlock aggregated into tipc_bearer. We want to reduce all
>> dependencies to this lock, as part of our effort to make the locking
>> policy simpler and more manageable.
>>
>> Furthermore, the functions tipc_disc_recv_msg() and disc_send_msg()
>> access the flag without taking this lock at all, implying a risk that
>> they may occasionally obtain a wrong value and drop sending/receiving
>> of discovery messages. Since discovery messages are timer driven and
>> best-effort, the latter problem is not fatal, but it is clearly avoidable.
>>
>> By converting the type of the flag from int to atomic_t we eliminate
>> both the spinlock dependency and the current race condition.
>>
>> We also make two small code changes:
>>
>> 1) The functions tipc_continue() and bearer_blocked() now become
>> one-liners doing a single access to the flag. So we just remove
>> those redundant functions.
>>
>> 2) The initialization of the flag is moved from the enable_media()
>> functions in ib_media.c and eth_media.c respecively, to the generic
>> tipc_enable_bearer() function, so that it is done in one place only.
>
> Maybe it is just me, but I still fail to see the benefit (or harm) of this change.
>
> As I understand it, the time line for the current implementation, and the
> one you are suggesting, look like this:
>
> Without atomc_t:
> ------------------------
> 1: Device driver detects loss of carrier or similar.
> 2: It issues a call (work_queue?) to all registered users.
> 3: Until the call has reached the users, they (including TIPC) will continue
> to send packets, although the driver can only drop them.
> 4: The field bearer->blocked is set as a one single write both on 32-bits and
> 64-bist cPUs, since it is one word, and a small constant value (1 or 0) that is written.
> 5: Other cores happening to work on the same cache line at that moment
> will notice nothing; they will continue to send the packet they were working
> on, seeing the previous value of the field.
> 6: The cache line is flushed and reloaded as soon as next packet is sent or received
> on these cores. (I don't think a single 64-byte cache line will remain between
> two transmissions. There is way to many data fields involved.)
> 7: The next time the cache line is reloaded it has the correct value of
> 'blocked', since the cache coherency HW support ensures that
> the cache is updated. Next packet is not sent.
>
>
> With atomic_t:
> ------------------
> 1: Device driver detects loss of carrier or similar.
> 2: It issues a call (work_queue?) to all registered users.
> 3: Until the call has reached the users, they (including TIPC) will continue
> to send packets, although the driver can only drop them.
> 4: The field bearer->blocked is set as a single, atomic write, which it would be
> anyway, just as in the previous case.
> 5: Other cores happening to read on the same cache line at that moment
> will now see the cache line invalidated, so they will have to reread it with the
> new, correct value. The 'volatile' keyword ensures that. As I understand
> GCC even puts in memory barrier around volatile variables, but I fail to
> to see the relevance here, we don make any other changes to the
> bearer struct anyway, so ordereing is not an issue.
> 6: The cache line now has the correct read value. Packet is not sent.
>
> What have we won? That TIPC may have time to send one more
> packet per core than it would otherwise, but nothing can stop
> TIPC and others from sending a small number of packets during the
> interval between carrier loss detection and until the 'blocked' flag is
> updated. The difference it makes if we cache line is invalidated immediately
> or at next reload is microscopic in this context.
>
> Admittedly, the harm done is not big either. The memory
> barriers put in by GCC around the volatile read does not cost much.
> But why make such a change if we cannot motivate it?
>
> Maybe my view is too simplistic, but then somebody has to explain
> what is wrong with this reasoning.
> Basically, I don't think we need to protect this field with neither
> spinlocks, memory barriers nor making it atomic.
>
First of all, thank you for so detailed comments. In fact the things you
concerned below often confuse me. So I spend some times trying to
understand memory cache system of modern processor as well as the
effects of kinds of Linux memory barrier primitives on memory cache
coherence. That's why I am late to respond you. But it's really so
complex that I am also afraid that I don't entirely understand them
although I consumed several days to research. Anyway, I am still going
to answer your questions :)
Nowadays most of modern processors have cache coherence protocols
ensuring that all CPUs maintain a consistent view of data. The most
famous coherence protocol is called MESI. For example, the blocked
variable is manipulated by core 0 and is only written back to its local
cache, and other cores snoop/monitor the change of core 0 from internal
high speed bus and then invalidate their local cacheline where the
blocked variable resides. When other cores read the blocked, they can
get the up-to-date value from the cacheline of core 0. The whole process
is guaranteed by MESI. However, why do we still need memory barriers if
processor HW has cache coherence capability? This is because these CPUs
cannot ensure the order of accessing memory occurring on core 0 is same
as happening on other cores although they can guarantee that data is
consistent. But in our case, as there is no required data dependency,
memory barriers seem useless for us in this case.
Unfortunately life doesn't sound like so easy! There are still some of
old CPUs being incapable of the cache coherence mechanisms, meaning that
manually flushing cache becomes mandatory for them. In this situation,
without memory barriers, the rate of getting stale data might
significantly arise. Even if many packets are involved(ie, many memory
areas would be access via CPU's local cache), in some extremely worst
case it doesn't mean the cacheline storing the stale blocked is
displaced/sacrificed within a short time. Therefore, it becomes
unpredictable for us when the stale blocked would be updated, which may
be unacceptable.
If we find barrier.h files in kernel source tree, below results are got:
*/net-next-2.6$ find ./ -name barrier.h
./arch/metag/include/asm/barrier.h
./arch/sparc/include/asm/barrier.h
./arch/h8300/include/asm/barrier.h
./arch/x86/um/asm/barrier.h
./arch/x86/include/asm/barrier.h
./arch/unicore32/include/asm/barrier.h
./arch/xtensa/include/asm/barrier.h
./arch/frv/include/asm/barrier.h
./arch/m68k/include/asm/barrier.h
./arch/hexagon/include/asm/barrier.h
./arch/mips/include/asm/barrier.h
./arch/arm64/include/asm/barrier.h
./arch/parisc/include/asm/barrier.h
./arch/s390/include/asm/barrier.h
./arch/ia64/include/asm/barrier.h
./arch/m32r/include/asm/barrier.h
./arch/powerpc/include/asm/barrier.h
./arch/alpha/include/asm/barrier.h
./arch/arm/include/asm/barrier.h
./arch/avr32/include/asm/barrier.h
./arch/sh/include/asm/barrier.h
./arch/microblaze/include/asm/barrier.h
./arch/mn10300/include/asm/barrier.h
./arch/tile/include/asm/barrier.h
./arch/arc/include/asm/barrier.h
./arch/score/include/asm/barrier.h
./arch/blackfin/include/asm/barrier.h
./arch/cris/include/asm/barrier.h
./include/asm-generic/barrier.h
Different of architectures have different implementations. Even if arch
is same, different HW versions also have different implementations. If
we take a closer look at them, it's found many of implementations
forcedly flush/sync cache in memory barriers operations.
Beside the effect on CPU cache, memory barriers also can suppress the
optimization of compiler. For it, I just attempt to state my
understanding of keyword - volatile, so please image below scenario:
CPU 0 CPU 1
----------- -----
T0: read blocked(blocked is 0 now)
T1: blocked = 1
T2: read blocked(blocked is 0)
As compiler optimizes the process of accessing blocked, without volatile
keyword decorating the variable of blocked, at T0, CPU 0 reads blocked
from main memory/cache and load it into one of its registers. At T1,
blocked is set to 1 by CPU 1. But as CPU 0 is unaware of the change, it
gets the value from its register instead of main memory or cache when it
reads blocked variable again at T2. We don't know when the value in the
register will be overwritten by other values, that means we don't know
when CPU 0 would read the true data. This may also be unacceptable for us.
Therefore, without memory barriers, in most time I believe everything
works fine. But to be more cautious, I still prefer to apply some
limitations to ensure blocked is always sync.
Of course, probably my above understanding is incomplete or incorrect.
If so, please correct me.
Regards,
Ying
> Regards
> ///jon
>
>
>>
>> Signed-off-by: Ying Xue <yin...@wi...>
>> ---
>> net/tipc/bcast.c | 4 ++--
>> net/tipc/bearer.c | 33 ++++-----------------------------
>> net/tipc/bearer.h | 5 +----
>> net/tipc/discover.c | 5 +++--
>> net/tipc/eth_media.c | 9 ++++-----
>> net/tipc/ib_media.c | 9 ++++-----
>> net/tipc/link.c | 12 ++++++------
>> 7 files changed, 24 insertions(+), 53 deletions(-)
>>
>> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
>> index 716de1a..c78f30c 100644
>> --- a/net/tipc/bcast.c
>> +++ b/net/tipc/bcast.c
>> @@ -615,8 +615,8 @@ static int tipc_bcbearer_send(struct sk_buff *buf, struct tipc_bearer *unused1,
>> if (!p)
>> break; /* No more bearers to try */
>>
>> - if (tipc_bearer_blocked(p)) {
>> - if (!s || tipc_bearer_blocked(s))
>> + if (atomic_read(&p->blocked)) {
>> + if (!s || atomic_read(&s->blocked))
>> continue; /* Can't use either bearer */
>> b = s;
>> }
>> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
>> index 3f9707a..17f9824 100644
>> --- a/net/tipc/bearer.c
>> +++ b/net/tipc/bearer.c
>> @@ -275,31 +275,6 @@ void tipc_bearer_remove_dest(struct tipc_bearer *b_ptr, u32 dest)
>> tipc_disc_remove_dest(b_ptr->link_req);
>> }
>>
>> -/*
>> - * Interrupt enabling new requests after bearer blocking:
>> - * See bearer_send().
>> - */
>> -void tipc_continue(struct tipc_bearer *b)
>> -{
>> - spin_lock_bh(&b->lock);
>> - b->blocked = 0;
>> - spin_unlock_bh(&b->lock);
>> -}
>> -
>> -/*
>> - * tipc_bearer_blocked - determines if bearer is currently blocked
>> - */
>> -int tipc_bearer_blocked(struct tipc_bearer *b)
>> -{
>> - int res;
>> -
>> - spin_lock_bh(&b->lock);
>> - res = b->blocked;
>> - spin_unlock_bh(&b->lock);
>> -
>> - return res;
>> -}
>> -
>> /**
>> * tipc_enable_bearer - enable bearer with the given name
>> */
>> @@ -387,6 +362,8 @@ restart:
>>
>> b_ptr = &tipc_bearers[bearer_id];
>> strcpy(b_ptr->name, name);
>> + atomic_set(&b_ptr->blocked, 0);
>> + smp_mb();
>> res = m_ptr->enable_media(b_ptr);
>> if (res) {
>> pr_warn("Bearer <%s> rejected, enable failure (%d)\n",
>> @@ -429,8 +406,8 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
>>
>> read_lock_bh(&tipc_net_lock);
>> pr_info("Blocking bearer <%s>\n", b_ptr->name);
>> + atomic_add_unless(&b_ptr->blocked, 1, 1);
>> spin_lock_bh(&b_ptr->lock);
>> - b_ptr->blocked = 1;
>> list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
>> struct tipc_node *n_ptr = l_ptr->owner;
>>
>> @@ -455,8 +432,8 @@ static void bearer_disable(struct tipc_bearer *b_ptr)
>> struct tipc_link_req *temp_req;
>>
>> pr_info("Disabling bearer <%s>\n", b_ptr->name);
>> + atomic_add_unless(&b_ptr->blocked, 1, 1);
>> spin_lock_bh(&b_ptr->lock);
>> - b_ptr->blocked = 1;
>> b_ptr->media->disable_media(b_ptr);
>> list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
>> tipc_link_delete(l_ptr);
>> @@ -489,8 +466,6 @@ int tipc_disable_bearer(const char *name)
>> return res;
>> }
>>
>> -
>> -
>> void tipc_bearer_stop(void)
>> {
>> u32 i;
>> diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
>> index e5e04be..8695d4a 100644
>> --- a/net/tipc/bearer.h
>> +++ b/net/tipc/bearer.h
>> @@ -130,7 +130,7 @@ struct tipc_media {
>> struct tipc_bearer {
>> void *usr_handle; /* initalized by media */
>> u32 mtu; /* initalized by media */
>> - int blocked; /* initalized by media */
>> + atomic_t blocked;
>> struct tipc_media_addr addr; /* initalized by media */
>> char name[TIPC_MAX_BEARER_NAME];
>> spinlock_t lock;
>> @@ -164,8 +164,6 @@ int tipc_register_media(struct tipc_media *m_ptr);
>> void tipc_recv_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr);
>>
>> int tipc_block_bearer(struct tipc_bearer *b_ptr);
>> -void tipc_continue(struct tipc_bearer *tb_ptr);
>> -
>> int tipc_enable_bearer(const char *bearer_name, u32 disc_domain, u32 priority);
>> int tipc_disable_bearer(const char *name);
>>
>> @@ -194,7 +192,6 @@ void tipc_bearer_remove_dest(struct tipc_bearer *b_ptr, u32 dest);
>> struct tipc_bearer *tipc_bearer_find(const char *name);
>> struct tipc_bearer *tipc_bearer_find_interface(const char *if_name);
>> struct tipc_media *tipc_media_find(const char *name);
>> -int tipc_bearer_blocked(struct tipc_bearer *b_ptr);
>> void tipc_bearer_stop(void);
>>
>> /**
>> diff --git a/net/tipc/discover.c b/net/tipc/discover.c
>> index ecc758c..d8b49fb 100644
>> --- a/net/tipc/discover.c
>> +++ b/net/tipc/discover.c
>> @@ -239,7 +239,8 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr)
>> /* Accept discovery message & send response, if necessary */
>> link_fully_up = link_working_working(link);
>>
>> - if ((type == DSC_REQ_MSG) && !link_fully_up && !b_ptr->blocked) {
>> + if ((type == DSC_REQ_MSG) && !link_fully_up &&
>> + !atomic_read(&b_ptr->blocked)) {
>> rbuf = tipc_disc_init_msg(DSC_RESP_MSG, orig, b_ptr);
>> if (rbuf) {
>> tipc_bearer_send(b_ptr, rbuf, &media_addr);
>> @@ -293,7 +294,7 @@ void tipc_disc_remove_dest(struct tipc_link_req *req)
>> */
>> static void disc_send_msg(struct tipc_link_req *req)
>> {
>> - if (!req->bearer->blocked)
>> + if (!atomic_read(&req->bearer->blocked))
>> tipc_bearer_send(req->bearer, req->buf, &req->dest);
>> }
>>
>> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
>> index f80d59f..32d01e8 100644
>> --- a/net/tipc/eth_media.c
>> +++ b/net/tipc/eth_media.c
>> @@ -1,7 +1,7 @@
>> /*
>> * net/tipc/eth_media.c: Ethernet bearer support for TIPC
>> *
>> - * Copyright (c) 2001-2007, Ericsson AB
>> + * Copyright (c) 2001-2007, 2013, Ericsson AB
>> * Copyright (c) 2005-2008, 2011-2013, Wind River Systems
>> * All rights reserved.
>> *
>> @@ -199,7 +199,6 @@ static int enable_media(struct tipc_bearer *tb_ptr)
>> tb_ptr->bcast_addr.media_id = TIPC_MEDIA_TYPE_ETH;
>> tb_ptr->bcast_addr.broadcast = 1;
>> tb_ptr->mtu = dev->mtu;
>> - tb_ptr->blocked = 0;
>> eth_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr);
>> return 0;
>> }
>> @@ -263,12 +262,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
>> switch (evt) {
>> case NETDEV_CHANGE:
>> if (netif_carrier_ok(dev))
>> - tipc_continue(eb_ptr->bearer);
>> + atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
>> else
>> tipc_block_bearer(eb_ptr->bearer);
>> break;
>> case NETDEV_UP:
>> - tipc_continue(eb_ptr->bearer);
>> + atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
>> break;
>> case NETDEV_DOWN:
>> tipc_block_bearer(eb_ptr->bearer);
>> @@ -276,7 +275,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
>> case NETDEV_CHANGEMTU:
>> case NETDEV_CHANGEADDR:
>> tipc_block_bearer(eb_ptr->bearer);
>> - tipc_continue(eb_ptr->bearer);
>> + atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
>> break;
>> case NETDEV_UNREGISTER:
>> case NETDEV_CHANGENAME:
>> diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
>> index c139892..5241a5b 100644
>> --- a/net/tipc/ib_media.c
>> +++ b/net/tipc/ib_media.c
>> @@ -5,7 +5,7 @@
>> *
>> * Based on eth_media.c, which carries the following copyright notice:
>> *
>> - * Copyright (c) 2001-2007, Ericsson AB
>> + * Copyright (c) 2001-2007, 2013, Ericsson AB
>> * Copyright (c) 2005-2008, 2011, Wind River Systems
>> * All rights reserved.
>> *
>> @@ -192,7 +192,6 @@ static int enable_media(struct tipc_bearer *tb_ptr)
>> tb_ptr->bcast_addr.media_id = TIPC_MEDIA_TYPE_IB;
>> tb_ptr->bcast_addr.broadcast = 1;
>> tb_ptr->mtu = dev->mtu;
>> - tb_ptr->blocked = 0;
>> ib_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr);
>> return 0;
>> }
>> @@ -256,12 +255,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
>> switch (evt) {
>> case NETDEV_CHANGE:
>> if (netif_carrier_ok(dev))
>> - tipc_continue(ib_ptr->bearer);
>> + atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
>> else
>> tipc_block_bearer(ib_ptr->bearer);
>> break;
>> case NETDEV_UP:
>> - tipc_continue(ib_ptr->bearer);
>> + atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
>> break;
>> case NETDEV_DOWN:
>> tipc_block_bearer(ib_ptr->bearer);
>> @@ -269,7 +268,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
>> case NETDEV_CHANGEMTU:
>> case NETDEV_CHANGEADDR:
>> tipc_block_bearer(ib_ptr->bearer);
>> - tipc_continue(ib_ptr->bearer);
>> + atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
>> break;
>> case NETDEV_UNREGISTER:
>> case NETDEV_CHANGENAME:
>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>> index 4d1297e..8c0e9a3 100644
>> --- a/net/tipc/link.c
>> +++ b/net/tipc/link.c
>> @@ -796,7 +796,7 @@ int tipc_link_send_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
>> return link_send_long_buf(l_ptr, buf);
>>
>> /* Packet can be queued or sent. */
>> - if (likely(!tipc_bearer_blocked(l_ptr->b_ptr) &&
>> + if (likely(!atomic_read(&l_ptr->b_ptr->blocked) &&
>> !link_congested(l_ptr))) {
>> link_add_to_outqueue(l_ptr, buf, msg);
>>
>> @@ -963,7 +963,7 @@ static int link_send_buf_fast(struct tipc_link *l_ptr, struct sk_buff *buf,
>>
>> if (likely(!link_congested(l_ptr))) {
>> if (likely(msg_size(msg) <= l_ptr->max_pkt)) {
>> - if (likely(!tipc_bearer_blocked(l_ptr->b_ptr))) {
>> + if (likely(!atomic_read(&l_ptr->b_ptr->blocked))) {
>> link_add_to_outqueue(l_ptr, buf, msg);
>> tipc_bearer_send(l_ptr->b_ptr, buf,
>> &l_ptr->media_addr);
>> @@ -1020,7 +1020,7 @@ exit:
>>
>> /* Exit if link (or bearer) is congested */
>> if (link_congested(l_ptr) ||
>> - tipc_bearer_blocked(l_ptr->b_ptr)) {
>> + atomic_read(&l_ptr->b_ptr->blocked)) {
>> res = link_schedule_port(l_ptr,
>> sender->ref, res);
>> goto exit;
>> @@ -1287,7 +1287,7 @@ void tipc_link_push_queue(struct tipc_link *l_ptr)
>> {
>> u32 res;
>>
>> - if (tipc_bearer_blocked(l_ptr->b_ptr))
>> + if (atomic_read(&l_ptr->b_ptr->blocked))
>> return;
>>
>> do {
>> @@ -1376,7 +1376,7 @@ void tipc_link_retransmit(struct tipc_link *l_ptr, struct sk_buff *buf,
>>
>> msg = buf_msg(buf);
>>
>> - if (tipc_bearer_blocked(l_ptr->b_ptr)) {
>> + if (atomic_read(&l_ptr->b_ptr->blocked)) {
>> if (l_ptr->retransm_queue_size == 0) {
>> l_ptr->retransm_queue_head = msg_seqno(msg);
>> l_ptr->retransm_queue_size = retransmits;
>> @@ -1870,7 +1870,7 @@ void tipc_link_send_proto_msg(struct tipc_link *l_ptr, u32 msg_typ,
>> buf->priority = TC_PRIO_CONTROL;
>>
>> /* Defer message if bearer is already blocked */
>> - if (tipc_bearer_blocked(l_ptr->b_ptr)) {
>> + if (atomic_read(&l_ptr->b_ptr->blocked)) {
>> l_ptr->proto_msg_queue = buf;
>> return;
>> }
>
>
>
|
|
From: jason <huz...@gm...> - 2013-10-25 03:20:28
|
Hi Ying,
As I under standing of the memory barrier, it is used for other CPU to see
a same order of two instructions (one before the memory barrier and one
after it) operating on memory as the local CPU. So if one decide to use
memory barrier, there must be at lease two memory operations first. In
your patch, operation on 'block' is just a single operation on memory which
just store a value to a memory location, no subsequent memory operations
presented to be execute in a strong order (must take effect on memory AFTER
set the 'block' flag) among all other CPUs. So I here agree on Jon too, no
need to use atomic, memory barrier and locks.
On Friday, October 25, 2013, Ying Xue wrote:
> On 10/21/2013 05:14 AM, Jon Maloy wrote:
> > On 09/30/2013 10:38 AM, Ying Xue wrote:
> >> The 'blocked' flag in struct tipc_bearer is currently protected by
> >> a big spinlock aggregated into tipc_bearer. We want to reduce all
> >> dependencies to this lock, as part of our effort to make the locking
> >> policy simpler and more manageable.
> >>
> >> Furthermore, the functions tipc_disc_recv_msg() and disc_send_msg()
> >> access the flag without taking this lock at all, implying a risk that
> >> they may occasionally obtain a wrong value and drop sending/receiving
> >> of discovery messages. Since discovery messages are timer driven and
> >> best-effort, the latter problem is not fatal, but it is clearly
> avoidable.
> >>
> >> By converting the type of the flag from int to atomic_t we eliminate
> >> both the spinlock dependency and the current race condition.
> >>
> >> We also make two small code changes:
> >>
> >> 1) The functions tipc_continue() and bearer_blocked() now become
> >> one-liners doing a single access to the flag. So we just remove
> >> those redundant functions.
> >>
> >> 2) The initialization of the flag is moved from the enable_media()
> >> functions in ib_media.c and eth_media.c respecively, to the generic
> >> tipc_enable_bearer() function, so that it is done in one place only.
> >
> > Maybe it is just me, but I still fail to see the benefit (or harm) of
> this change.
> >
> > As I understand it, the time line for the current implementation, and the
> > one you are suggesting, look like this:
> >
> > Without atomc_t:
> > ------------------------
> > 1: Device driver detects loss of carrier or similar.
> > 2: It issues a call (work_queue?) to all registered users.
> > 3: Until the call has reached the users, they (including TIPC) will
> continue
> > to send packets, although the driver can only drop them.
> > 4: The field bearer->blocked is set as a one single write both on
> 32-bits and
> > 64-bist cPUs, since it is one word, and a small constant value (1
> or 0) that is written.
> > 5: Other cores happening to work on the same cache line at that moment
> > will notice nothing; they will continue to send the packet they were
> working
> > on, seeing the previous value of the field.
> > 6: The cache line is flushed and reloaded as soon as next packet is sent
> or received
> > on these cores. (I don't think a single 64-byte cache line will
> remain between
> > two transmissions. There is way to many data fields involved.)
> > 7: The next time the cache line is reloaded it has the correct value of
> > 'blocked', since the cache coherency HW support ensures that
> > the cache is updated. Next packet is not sent.
> >
> >
> > With atomic_t:
> > ------------------
> > 1: Device driver detects loss of carrier or similar.
> > 2: It issues a call (work_queue?) to all registered users.
> > 3: Until the call has reached the users, they (including TIPC) will
> continue
> > to send packets, although the driver can only drop them.
> > 4: The field bearer->blocked is set as a single, atomic write, which it
> would be
> > anyway, just as in the previous case.
> > 5: Other cores happening to read on the same cache line at that moment
> > will now see the cache line invalidated, so they will have to reread
> it with the
> > new, correct value. The 'volatile' keyword ensures that. As I
> understand
> > GCC even puts in memory barrier around volatile variables, but I
> fail to
> > to see the relevance here, we don make any other changes to the
> > bearer struct anyway, so ordereing is not an issue.
> > 6: The cache line now has the correct read value. Packet is not sent.
> >
> > What have we won? That TIPC may have time to send one more
> > packet per core than it would otherwise, but nothing can stop
> > TIPC and others from sending a small number of packets during the
> > interval between carrier loss detection and until the 'blocked' flag is
> > updated. The difference it makes if we cache line is invalidated
> immediately
> > or at next reload is microscopic in this context.
> >
> > Admittedly, the harm done is not big either. The memory
> > barriers put in by GCC around the volatile read does not cost much.
> > But why make such a change if we cannot motivate it?
> >
> > First of all, thank you for so detailed comments. In fact the things you
> concerned below often confuse me. So I spend some times trying to
> understand memory cache system of modern processor as well as the
> effects of kinds of Linux memory barrier primitives on memory cache
> coherence. That's why I am late to respond you. But it's really so
> complex that I am also afraid that I don't entirely understand them
> although I consumed several days to research. Anyway, I am still going
> to answer your questions :)
>
> Nowadays most of modern processors have cache coherence protocols
> ensuring that all CPUs maintain a consistent view of data. The most
> famous coherence protocol is called MESI. For example, the blocked
> variable is manipulated by core 0 and is only written back to its local
> cache, and other cores snoop/monitor the change of core 0 from internal
> high speed bus and then invalidate their local cacheline where the
> blocked variable resides. When other cores read the blocked, they can
> get the up-to-date value from the cacheline of core 0. The whole process
> is guaranteed by MESI. However, why do we still need memory barriers if
> processor HW has cache coherence capability? This is because these CPUs
> cannot ensure the order of accessing memory occurring on core 0 is same
> as happening on other cores although they can guarantee that data is
> consistent. But in our case, as there is no required data dependency,
> memory barriers seem useless for us in this case.
>
> Unfortunately life doesn't sound like so easy! There are still some of
> old CPUs being incapable of the cache coherence mechanisms, meaning that
> manually flushing cache becomes mandatory for them. In this situation,
> without memory barriers, the rate of getting stale data might
> significantly arise. Even if many packets are involved(ie, many memory
> areas would be access via CPU's local cache), in some extremely worst
> case it doesn't mean the cacheline storing the stale blocked is
> displaced/sacrificed within a short time. Therefore, it becomes
> unpredictable for us when the stale blocked would be updated, which may
> be unacceptable.
>
> If we find barrier.h files in kernel source tree, below results are got:
>
> */net-next-2.6$ find ./ -name barrier.h
> ./arch/metag/include/asm/barrier.h
> ./arch/sparc/include/asm/barrier.h
> ./arch/h8300/include/asm/barrier.h
> ./arch/x86/um/asm/barrier.h
> ./arch/x86/include/asm/barrier.h
> ./arch/unicore32/include/asm/barrier.h
> ./arch/xtensa/include/asm/barrier.h
> ./arch/frv/include/asm/barrier.h
> ./arch/m68k/include/asm/barrier.h
> ./arch/hexagon/include/asm/barrier.h
> ./arch/mips/include/asm/barrier.h
> ./arch/arm64/include/asm/barrier.h
> ./arch/parisc/include/asm/barrier.h
> ./arch/s390/include/asm/barrier.h
> ./arch/ia64/include/asm/barrier.h
> ./arch/m32r/include/asm/barrier.h
> ./arch/powerpc/include/asm/barrier.h
> ./arch/alpha/include/asm/barrier.h
> ./arch/arm/include/asm/barrier.h
> ./arch/avr32/include/asm/barrier.h
> ./arch/sh/include/asm/barrier.h
> ./arch/microblaze/include/asm/barrier.h
> ./arch/mn10300/include/asm/barrier.h
> ./arch/tile/include/asm/barrier.h
> ./arch/arc/include/asm/barrier.h
> ./arch/score/include/asm/barrier.h
> ./arch/blackfin/include/asm/barrier.h
> ./arch/cris/include/asm/barrier.h
> ./include/asm-generic/barrier.h
>
> Different of architectures have different implementations. Even if arch
> is same, different HW versions also have different implementations. If
> we take a closer look at them, it's found many of implementations
> forcedly flush/sync cache in memory barriers operations.
>
> Beside the effect on CPU cache, memory barriers also can suppress the
> optimization of compiler. For it, I just attempt to state my
> understanding of keyword - volatile, so please image below scenario:
>
> CPU 0 CPU 1
> ----------- -----
> T0: read blocked(blocked is 0 now)
> T1: blocked = 1
> T2: read blocked(blocked is 0)
>
> As compiler optimizes the process of accessing blocked, without volatile
> keyword decorating the variable of blocked, at T0, CPU 0 reads blocked
> from main memory/cache and load it into one of its registers. At T1,
> blocked is set to 1 by CPU 1. But as CPU 0 is unaware of the change, it
> gets the value from its register instead of main memory or cache when it
> reads blocked variable again at T2. We don't know when the value in the
> register will be overwritten by other values, that means we don't know
> when CPU 0 would read the true data. This may also be unacceptable for us.
>
> Therefore, without memory barriers, in most time I believe everything
> works fine. But to be more cautious, I still prefer to apply some
> limitations to ensure blocked is always sync.
>
> Of course, probably my above understanding is incomplete or incorrect.
> If so, please correct me.
>
>
> Regards,
> Ying
>
>
> > Regards
> > ///jon
> >
> >
> >>
> >> Signed-off-by: Ying Xue <yin...@wi...>
> >> ---
> >> net/tipc/bcast.c | 4 ++--
> >> net/tipc/bearer.c | 33 ++++-----------------------------
> >> net/tipc/bearer.h | 5 +----
> >> net/tipc/discover.c | 5 +++--
> >> net/tipc/eth_media.c | 9 ++++-----
> >> net/tipc/ib_media.c | 9 ++++-----
> >> net/tipc/link.c | 12 ++++++------
> >> 7 files changed, 24 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> >> index 716de1a..c78f30c 100644
> >> --- a/net/tipc/bcast.c
> >> +++ b/net/tipc/bcast.c
> >> @@ -615,8 +615,8 @@ static int tipc_bcbearer_send(struct sk_buff *buf,
> struct tipc_bearer *unused1,
> >> if (!p)
> >> break; /* No more bearers to try */
> >>
> >> - if (tipc_bearer_blocked(p)) {
> >> - if (!s || tipc_bearer_blocked(s))
> >> + if (atomic_read(&p->blocked)) {
> >> + if (!s || atomic_read(&s->blocked))
> >> continue; /* Can't use either bearer */
> >> b = s;
> >> }
> >> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> >> index 3f9707a..17f9824 100644
> >> --- a/net/tipc/bearer.c
> >> +++ b/net/tipc/bearer.c
> >> @@ -275,31 +275,6 @@ void tipc_bearer_remove_dest(struct tipc_bearer
> *b_ptr, u32 dest)
> >> tipc_disc_remove_dest(b_ptr->link_req);
> >> }
> >>
> >> -/*
> >> - * Interrupt enabling new requests after bearer blocking:
> >> - * See bearer_send().
> >> - */
> >> -void tipc_continue(struct tipc_bearer *b)
> >> -{
> >> - spin_lock_bh(&b->lock);
> >> - b->blocked = 0;
> >> - spin_unlock_bh(&b->lock);
> >> -}
> >> -
> >> -/*
> >> - * tipc_bearer_blocked - determines if bearer is currently blocked
> >> - */
> >> -int tipc_bearer_blocked(struct tipc_bearer *b)
> >> -{
> >> - int res;
> >> -
> >> - spin_lock_bh(&b->lock);
> >> - res = b->blocked;
> >> - spin_unlock_bh(&b->lock);
> >> -
> >> - return res;
> >> -}
> >> -
> >> /**
> >> * tipc_enable_bearer - enable bearer with the given name
> >> */
> >> @@ -387,6 +362,8 @@ restart:
> >>
> >> b_ptr = &tipc_bearers[bearer_id];
> >> strcpy(b_ptr->name, name);
> >> + atomic_set(&b_ptr->blocked, 0);
> >> + smp_mb();
> >> res = m_ptr->enable_media(b_ptr);
> >> if (res) {
> >> pr_warn("Bearer <%s> rejected, enable failure (%d)\n",
> >> @@ -429,8 +406,8 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
> >>
> >> read_lock_bh(&tipc_net_lock);
> >> pr_info("Blocking bearer <%s>\n", b_ptr->name);
> >> + atomic_add_unless(&b_ptr->blocked, 1, 1);
> >> spin_lock_bh(&b_ptr->lock);
> >> - b_ptr->blocked = 1;
> >> list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links,
> link_list) {
> >> struct tipc_node *n_ptr = l_ptr->owner;
> >>
> >> @@ -455,8 +432,8 @@ static void bearer_disable(struct tipc_bearer
> *b_ptr)
> >> struct tipc_link_req *temp_req;
> >>
> http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
> _______________________________________________
> tipc-discussion mailing list
> tip...@li... <javascript:;>
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>
--
Yours,
Jason
|
|
From: Ying X. <yin...@wi...> - 2013-10-25 03:32:28
|
On 10/25/2013 11:20 AM, jason wrote:
> Hi Ying,
> As I under standing of the memory barrier, it is used for other CPU to
> see a same order of two instructions (one before the memory barrier and
> one after it) operating on memory as the local CPU. So if one decide to
> use memory barrier, there must be at lease two memory operations first.
> In your patch, operation on 'block' is just a single operation on memory
> which just store a value to a memory location, no subsequent memory
> operations presented to be execute in a strong order (must take effect
> on memory AFTER set the 'block' flag) among all other CPUs. So I here
> agree on Jon too, no need to use atomic, memory barrier and locks.
>
Please carefully read my mail before you post your comments.
I have placed emphasis which there is no required data dependency in our
case. But I have listed two exceptions: one is that some older
processors have weak or even no cache coherences system. Specially you
can look through implementations of memory barriers in all archs.
Another case is compiler optimization factor.
//Ying
> On Friday, October 25, 2013, Ying Xue wrote:
>
> On 10/21/2013 05:14 AM, Jon Maloy wrote:
> > On 09/30/2013 10:38 AM, Ying Xue wrote:
> >> The 'blocked' flag in struct tipc_bearer is currently protected by
> >> a big spinlock aggregated into tipc_bearer. We want to reduce all
> >> dependencies to this lock, as part of our effort to make the locking
> >> policy simpler and more manageable.
> >>
> >> Furthermore, the functions tipc_disc_recv_msg() and disc_send_msg()
> >> access the flag without taking this lock at all, implying a risk that
> >> they may occasionally obtain a wrong value and drop sending/receiving
> >> of discovery messages. Since discovery messages are timer driven and
> >> best-effort, the latter problem is not fatal, but it is clearly
> avoidable.
> >>
> >> By converting the type of the flag from int to atomic_t we eliminate
> >> both the spinlock dependency and the current race condition.
> >>
> >> We also make two small code changes:
> >>
> >> 1) The functions tipc_continue() and bearer_blocked() now become
> >> one-liners doing a single access to the flag. So we just remove
> >> those redundant functions.
> >>
> >> 2) The initialization of the flag is moved from the enable_media()
> >> functions in ib_media.c and eth_media.c respecively, to the
> generic
> >> tipc_enable_bearer() function, so that it is done in one place
> only.
> >
> > Maybe it is just me, but I still fail to see the benefit (or harm)
> of this change.
> >
> > As I understand it, the time line for the current implementation,
> and the
> > one you are suggesting, look like this:
> >
> > Without atomc_t:
> > ------------------------
> > 1: Device driver detects loss of carrier or similar.
> > 2: It issues a call (work_queue?) to all registered users.
> > 3: Until the call has reached the users, they (including TIPC)
> will continue
> > to send packets, although the driver can only drop them.
> > 4: The field bearer->blocked is set as a one single write both on
> 32-bits and
> > 64-bist cPUs, since it is one word, and a small constant
> value (1 or 0) that is written.
> > 5: Other cores happening to work on the same cache line at that moment
> > will notice nothing; they will continue to send the packet
> they were working
> > on, seeing the previous value of the field.
> > 6: The cache line is flushed and reloaded as soon as next packet
> is sent or received
> > on these cores. (I don't think a single 64-byte cache line
> will remain between
> > two transmissions. There is way to many data fields involved.)
> > 7: The next time the cache line is reloaded it has the correct
> value of
> > 'blocked', since the cache coherency HW support ensures that
> > the cache is updated. Next packet is not sent.
> >
> >
> > With atomic_t:
> > ------------------
> > 1: Device driver detects loss of carrier or similar.
> > 2: It issues a call (work_queue?) to all registered users.
> > 3: Until the call has reached the users, they (including TIPC)
> will continue
> > to send packets, although the driver can only drop them.
> > 4: The field bearer->blocked is set as a single, atomic write,
> which it would be
> > anyway, just as in the previous case.
> > 5: Other cores happening to read on the same cache line at that moment
> > will now see the cache line invalidated, so they will have to
> reread it with the
> > new, correct value. The 'volatile' keyword ensures that. As I
> understand
> > GCC even puts in memory barrier around volatile variables, but
> I fail to
> > to see the relevance here, we don make any other changes to the
> > bearer struct anyway, so ordereing is not an issue.
> > 6: The cache line now has the correct read value. Packet is not sent.
> >
> > What have we won? That TIPC may have time to send one more
> > packet per core than it would otherwise, but nothing can stop
> > TIPC and others from sending a small number of packets during the
> > interval between carrier loss detection and until the 'blocked'
> flag is
> > updated. The difference it makes if we cache line is invalidated
> immediately
> > or at next reload is microscopic in this context.
> >
> > Admittedly, the harm done is not big either. The memory
> > barriers put in by GCC around the volatile read does not cost much.
> > But why make such a change if we cannot motivate it?
> >
> > First of all, thank you for so detailed comments. In fact the
> things you
> concerned below often confuse me. So I spend some times trying to
> understand memory cache system of modern processor as well as the
> effects of kinds of Linux memory barrier primitives on memory cache
> coherence. That's why I am late to respond you. But it's really so
> complex that I am also afraid that I don't entirely understand them
> although I consumed several days to research. Anyway, I am still going
> to answer your questions :)
>
> Nowadays most of modern processors have cache coherence protocols
> ensuring that all CPUs maintain a consistent view of data. The most
> famous coherence protocol is called MESI. For example, the blocked
> variable is manipulated by core 0 and is only written back to its local
> cache, and other cores snoop/monitor the change of core 0 from internal
> high speed bus and then invalidate their local cacheline where the
> blocked variable resides. When other cores read the blocked, they can
> get the up-to-date value from the cacheline of core 0. The whole process
> is guaranteed by MESI. However, why do we still need memory barriers if
> processor HW has cache coherence capability? This is because these CPUs
> cannot ensure the order of accessing memory occurring on core 0 is same
> as happening on other cores although they can guarantee that data is
> consistent. But in our case, as there is no required data dependency,
> memory barriers seem useless for us in this case.
>
> Unfortunately life doesn't sound like so easy! There are still some of
> old CPUs being incapable of the cache coherence mechanisms, meaning that
> manually flushing cache becomes mandatory for them. In this situation,
> without memory barriers, the rate of getting stale data might
> significantly arise. Even if many packets are involved(ie, many memory
> areas would be access via CPU's local cache), in some extremely worst
> case it doesn't mean the cacheline storing the stale blocked is
> displaced/sacrificed within a short time. Therefore, it becomes
> unpredictable for us when the stale blocked would be updated, which may
> be unacceptable.
>
> If we find barrier.h files in kernel source tree, below results are got:
>
> */net-next-2.6$ find ./ -name barrier.h
> ./arch/metag/include/asm/barrier.h
> ./arch/sparc/include/asm/barrier.h
> ./arch/h8300/include/asm/barrier.h
> ./arch/x86/um/asm/barrier.h
> ./arch/x86/include/asm/barrier.h
> ./arch/unicore32/include/asm/barrier.h
> ./arch/xtensa/include/asm/barrier.h
> ./arch/frv/include/asm/barrier.h
> ./arch/m68k/include/asm/barrier.h
> ./arch/hexagon/include/asm/barrier.h
> ./arch/mips/include/asm/barrier.h
> ./arch/arm64/include/asm/barrier.h
> ./arch/parisc/include/asm/barrier.h
> ./arch/s390/include/asm/barrier.h
> ./arch/ia64/include/asm/barrier.h
> ./arch/m32r/include/asm/barrier.h
> ./arch/powerpc/include/asm/barrier.h
> ./arch/alpha/include/asm/barrier.h
> ./arch/arm/include/asm/barrier.h
> ./arch/avr32/include/asm/barrier.h
> ./arch/sh/include/asm/barrier.h
> ./arch/microblaze/include/asm/barrier.h
> ./arch/mn10300/include/asm/barrier.h
> ./arch/tile/include/asm/barrier.h
> ./arch/arc/include/asm/barrier.h
> ./arch/score/include/asm/barrier.h
> ./arch/blackfin/include/asm/barrier.h
> ./arch/cris/include/asm/barrier.h
> ./include/asm-generic/barrier.h
>
> Different of architectures have different implementations. Even if arch
> is same, different HW versions also have different implementations. If
> we take a closer look at them, it's found many of implementations
> forcedly flush/sync cache in memory barriers operations.
>
> Beside the effect on CPU cache, memory barriers also can suppress the
> optimization of compiler. For it, I just attempt to state my
> understanding of keyword - volatile, so please image below scenario:
>
> CPU 0 CPU 1
> ----------- -----
> T0: read blocked(blocked is 0 now)
> T1: blocked = 1
> T2: read blocked(blocked is 0)
>
> As compiler optimizes the process of accessing blocked, without volatile
> keyword decorating the variable of blocked, at T0, CPU 0 reads blocked
> from main memory/cache and load it into one of its registers. At T1,
> blocked is set to 1 by CPU 1. But as CPU 0 is unaware of the change, it
> gets the value from its register instead of main memory or cache when it
> reads blocked variable again at T2. We don't know when the value in the
> register will be overwritten by other values, that means we don't know
> when CPU 0 would read the true data. This may also be unacceptable
> for us.
>
> Therefore, without memory barriers, in most time I believe everything
> works fine. But to be more cautious, I still prefer to apply some
> limitations to ensure blocked is always sync.
>
> Of course, probably my above understanding is incomplete or incorrect.
> If so, please correct me.
>
>
> Regards,
> Ying
>
>
> > Regards
> > ///jon
> >
> >
> >>
> >> Signed-off-by: Ying Xue <yin...@wi...>
> >> ---
> >> net/tipc/bcast.c | 4 ++--
> >> net/tipc/bearer.c | 33 ++++-----------------------------
> >> net/tipc/bearer.h | 5 +----
> >> net/tipc/discover.c | 5 +++--
> >> net/tipc/eth_media.c | 9 ++++-----
> >> net/tipc/ib_media.c | 9 ++++-----
> >> net/tipc/link.c | 12 ++++++------
> >> 7 files changed, 24 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> >> index 716de1a..c78f30c 100644
> >> --- a/net/tipc/bcast.c
> >> +++ b/net/tipc/bcast.c
> >> @@ -615,8 +615,8 @@ static int tipc_bcbearer_send(struct sk_buff
> *buf, struct tipc_bearer *unused1,
> >> if (!p)
> >> break; /* No more bearers to try */
> >>
> >> - if (tipc_bearer_blocked(p)) {
> >> - if (!s || tipc_bearer_blocked(s))
> >> + if (atomic_read(&p->blocked)) {
> >> + if (!s || atomic_read(&s->blocked))
> >> continue; /* Can't use either bearer */
> >> b = s;
> >> }
> >> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> >> index 3f9707a..17f9824 100644
> >> --- a/net/tipc/bearer.c
> >> +++ b/net/tipc/bearer.c
> >> @@ -275,31 +275,6 @@ void tipc_bearer_remove_dest(struct
> tipc_bearer *b_ptr, u32 dest)
> >> tipc_disc_remove_dest(b_ptr->link_req);
> >> }
> >>
> >> -/*
> >> - * Interrupt enabling new requests after bearer blocking:
> >> - * See bearer_send().
> >> - */
> >> -void tipc_continue(struct tipc_bearer *b)
> >> -{
> >> - spin_lock_bh(&b->lock);
> >> - b->blocked = 0;
> >> - spin_unlock_bh(&b->lock);
> >> -}
> >> -
> >> -/*
> >> - * tipc_bearer_blocked - determines if bearer is currently blocked
> >> - */
> >> -int tipc_bearer_blocked(struct tipc_bearer *b)
> >> -{
> >> - int res;
> >> -
> >> - spin_lock_bh(&b->lock);
> >> - res = b->blocked;
> >> - spin_unlock_bh(&b->lock);
> >> -
> >> - return res;
> >> -}
> >> -
> >> /**
> >> * tipc_enable_bearer - enable bearer with the given name
> >> */
> >> @@ -387,6 +362,8 @@ restart:
> >>
> >> b_ptr = &tipc_bearers[bearer_id];
> >> strcpy(b_ptr->name, name);
> >> + atomic_set(&b_ptr->blocked, 0);
> >> + smp_mb();
> >> res = m_ptr->enable_media(b_ptr);
> >> if (res) {
> >> pr_warn("Bearer <%s> rejected, enable failure (%d)\n",
> >> @@ -429,8 +406,8 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
> >>
> >> read_lock_bh(&tipc_net_lock);
> >> pr_info("Blocking bearer <%s>\n", b_ptr->name);
> >> + atomic_add_unless(&b_ptr->blocked, 1, 1);
> >> spin_lock_bh(&b_ptr->lock);
> >> - b_ptr->blocked = 1;
> >> list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links,
> link_list) {
> >> struct tipc_node *n_ptr = l_ptr->owner;
> >>
> >> @@ -455,8 +432,8 @@ static void bearer_disable(struct tipc_bearer
> *b_ptr)
> >> struct tipc_link_req *temp_req;
> >>
> http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
> _______________________________________________
> tipc-discussion mailing list
> tip...@li... <javascript:;>
> https://lists.sourceforge.net/lists/listinfo/tipc-discussion
>
>
>
> --
> Yours,
> Jason
|
|
From: jason <huz...@gm...> - 2013-10-25 07:49:06
|
Hi Ying,
On Friday, October 25, 2013, Ying Xue wrote:
> On 10/25/2013 11:20 AM, jason wrote:
> > Hi Ying,
> > As I under standing of the memory barrier, it is used for other CPU to
> > see a same order of two instructions (one before the memory barrier and
> > one after it) operating on memory as the local CPU. So if one decide to
> > use memory barrier, there must be at lease two memory operations first.
> > In your patch, operation on 'block' is just a single operation on memory
> > which just store a value to a memory location, no subsequent memory
> > operations presented to be execute in a strong order (must take effect
> > on memory AFTER set the 'block' flag) among all other CPUs. So I here
> > agree on Jon too, no need to use atomic, memory barrier and locks.
> >
>
> Please carefully read my mail before you post your comments.
>
> I have placed emphasis which there is no required data dependency in our
> case. But I have listed two exceptions: one is that some older
> processors have weak or even no cache coherences system. Specially you
> can look through implementations of memory barriers in all archs.
Sorry for my careless...
Yes, I got your point ,and you are correct absolutely. Yes, many
architectures do need memory barrier to force to update cache, but in most
case, you do not need to do it explicitly. Many kinds of implicit memory
barrier will appear in the subsequent code, such as a
rtnl_lock/rtnl_unlock... If you can find one, then your memory barrier
operation can be omitted and improve performance.
> Another case is compiler optimization factor.
>
>
Yes, memory barrier can suppress compiler optimization on the reader side.
But I think as you know, a volatile key word is enough for this job.
>
> //Ying
>
> > On Friday, October 25, 2013, Ying Xue wrote:
> >
> > On 10/21/2013 05:14 AM, Jon Maloy wrote:
> > > On 09/30/2013 10:38 AM, Ying Xue wrote:
> > >> The 'blocked' flag in struct tipc_bearer is currently protected by
> > >> a big spinlock aggregated into tipc_bearer. We want to reduce all
> > >> dependencies to this lock, as part of our effort to make the
> locking
> > >> policy simpler and more manageable.
> > >>
> > >> Furthermore, the functions tipc_disc_recv_msg() and
> disc_send_msg()
> > >> access the flag without taking this lock at all, implying a risk
> that
> > >> they may occasionally obtain a wrong value and drop
> sending/receiving
> > >> of discovery messages. Since discovery messages are timer driven
> and
> > >> best-effort, the latter problem is not fatal, but it is clearly
> > avoidable.
> > >>
> > >> By converting the type of the flag from int to atomic_t we
> eliminate
> > >> both the spinlock dependency and the current race condition.
> > >>
> > >> We also make two small code changes:
> > >>
> > >> 1) The functions tipc_continue() and bearer_blocked() now become
> > >> one-liners doing a single access to the flag. So we just remove
> > >> those redundant functions.
> > >>
> > >> 2) The initialization of the flag is moved from the enable_media()
> > >> functions in ib_media.c and eth_media.c respecively, to the
> > generic
> > >> tipc_enable_bearer() function, so that it is done in one place
> > only.
> > >
> > > Maybe it is just me, but I still fail to see the benefit (or harm)
> > of this change.
> > >
> > > As I understand it, the time line for the current implementation,
> > and the
> > > one you are suggesting, look like this:
> > >
> > > Without atomc_t:
> > > ------------------------
> > > 1: Device driver detects loss of carrier or similar.
> > > 2: It issues a call (work_queue?) to all registered users.
> > > 3: Until the call has reached the users, they (including TIPC)
> > will continue
> > > to send packets, although the driver can only drop them.
> > > 4: The field bearer->blocked is set as a one single write both on
> > 32-bits and
> > > 64-bist cPUs, since it is one word, and a small constant
> > value (1 or 0) that is written.
> > > 5: Other cores happening to work on the same cache line at that
> moment
> > > will notice nothing; they will continue to send the packet
> > they were working
> > > on, seeing the previous value of the field.
> > > 6: The cache line is flushed and reloaded as soon as next packet
> > is sent or received
> > > on these cores. (I don't think a single 64-byte cache line
> > will remain between
> > > two transmissions. There is way to many data fields involved.)
> > > 7: The next time the cache line is reloaded it has the correct
> > value of
> > > 'blocked', since the cache coherency HW support ensures that
> > > the cache is updated. Next packet is not sent.
> > >
> > >
> > > With atomic_t:
> > > ------------------
> > > 1: Device driver detects loss of carrier or similar.
> > > 2: It issues a call (work_queue?) to all registered users.
> > > 3: Until the call has reached the users, they (including TIPC)
> > will continue
> > > to send packets, although the driver can only drop them.
> > > 4: The field bearer->blocked is set as a single, atomic write,
> > which it would be
> > > anyway, just as in the previous case.
> > > 5: Other cores happening to read on the same cache line at that
> moment
> > > will now see the cache line invalidated, so they will have to
> > reread it with the
> > > new, correct value. The 'volatile' keyword ensures that. As I
> > understand
> > > GCC even puts in memory barrier around volatile variables, but
> > I fail to
> > > to see the relevance here, we don make any other changes to the
> > > bearer struct anyway, so ordereing is not an issue.
> > > 6: The cache line now has the correct read value. Packet is not
> sent.
> > >
> > > What have we won? That TIPC may have time to send one more
> > > packet per core than it would otherwise, but nothing can stop
> > > TIPC and others from sending a small number of packets during the
> > > interval between carrier loss detection and until the 'blocked'
> > flag is
> > > updated. The difference it makes if we cache line is invalidated
> > immediately
> > > or at next reload is microscopic in this context.
> > >
> > > Admittedly, the harm done is not big either. The memory
> > > barriers put in by GCC around the volatile read does not cost much.
> > > But why make such a change if we cannot motivate it?
> > >
> > > First of all, thank you for so detailed comments. In fact the
> > things you
> > concerned below often confuse me. So I spend some times trying to
> > understand memory cache system of modern processor as well as the
> > effects of kinds of Linux memory barrier primitives on memory cache
> > coherence. That's why I am late to respond you. But it's really so
> > complex that I am also afraid that I don't entirely understand them
> > although I consumed several days to research. Anyway, I am still
> going
> > to answer your questions :)
> >
> > Nowadays most of modern processors have cache coherence protocols
> > ensuring that all CPUs maintain a consistent view of data. The most
> > famous coherence protocol is called MESI. For example, the blocked
> > variable is manipulated by core 0 and is only written back to its
> local
> > cache, and other cores snoop/monitor the change of core 0 from
> internal
> > high speed bus and then invalidate their local cacheline where the
> > blocked variable resides. When other cores read the blocked, they can
> > get the up-to-date value from the cacheline of core 0. The whole
> process
> > is guaranteed by MESI. However, why do we still need memory barriers
> if
> > processor HW has cache coherence capability? This is because these
> CPUs
> > cannot ensure the order of accessing memory occurring on core 0 is
> same
> > as happening on other cores although they can guarantee that data is
> > consistent. But in our case, as there is no required data dependency,
> > memory barriers seem useless for us in this case.
> >
> > Unfortunately life doesn't sound like so easy! There are still some
> of
> > old CPUs being incapable of the cache coherence mechanisms, meaning
> that
> > manually flushing cache becomes mandatory for them. In this
> situation,
> > without memory barriers, the rate of getting stale data might
> > significantly arise. Even if many packets are involved(ie, many
> memory
> > areas would be access via CPU's local cache), in some extremely worst
> > case it doesn't mean the cacheline storing the stale blocked is
> > displaced/sacrificed within a short time. Therefore, it becomes
> > unpredictable for us when the stale blocked would be updated, which
> may
> > be unacceptable.
> >
> > If we find barrier.h files in kernel source tree, below results are
> got:
> >
> > */net-next-2.6$ find ./ -name barrier.h
> > ./arch/metag/include/asm/barrier.h
> > ./arch/sparc/include/asm/barrier.h
> > ./arch/h8300/include/asm/barrier.h
> > ./arch/x86/um/asm/barrier.h
> > ./arch/x86/include/asm/barrier.h
> > ./arch/unicore32/include/asm/barrier.h
> > ./arch/xtensa/include/asm/barrier.h
> > ./arch/frv/include/asm/barrier.h
> > ./arch/m68k/include/asm/barrier.h
> > ./arch/hexagon/include/asm/barrier.h
> > ./arch/mips/include/asm/barrier.h
> > ./arch/arm64/include/asm/barrier.h
> > ./arch/parisc/include/asm/barrier.h
> > ./arch/s390/include/asm/barrier.h
> > ./arch/ia64/include/asm/barrier.h
> > ./arch/m32r/include/asm/barrier.h
> > ./arch/powerpc/include/asm/barrier.h
> > ./arch/alpha/include/asm/barrier.h
> > ./arch/arm/include/asm/barrier.h
> > ./arch/avr32/include/asm/barrier.h
> > ./arch/sh/include/asm/barrier.h
> > ./arch/microblaze/include/asm/barrier.h
> > ./arch/mn10300/include/asm/barrier.h
> > ./arch/tile/include/asm/barrier.h
> > ./arch/arc/include/asm/barrier.h
> > ./arch/score/include/asm/barrier.h
> > ./arch/blackfin/include/asm/barrier.h
> > ./arch/cris/include/asm/barrier.h
> > ./include/asm-generic/barrier.h
> >
> > Different of architectures have different implementations. Even if
> arch
> > is same, different HW versions also have different implementations.
> If
> > we take a closer look at them, it's found many of implementations
> > forcedly flush/sync cache in memory barriers operations.
> >
> > Beside the effect on CPU cache, memory barriers also can suppress the
> > optimization of compiler. For it, I just attempt to state my
> > understanding of keyword - volatile, so please image below scenario:
> >
> > CPU 0 CPU 1
> > ----------- -----
> > T0: read blocked(blocked is 0 now)
> > T1: blocked = 1
> > T2: read blocked(blocked is 0)
> >
> > As compiler optimizes the process of accessing blocked, without
> volatile
> > keyword decorating the variable of blocked, at T0, CPU 0 reads
> blocked
> > from main memory/cache and load it into one of its registers. At T1,
> > blocked is set to 1 by CPU 1. But as CPU 0 is unaware of the change,
> it
> > gets the value from its register instead of main memory or cache
> when it
> > reads blocked variable again at T2. We don't know when the value in
> the
> > register will be overwritten by other values, that means we don't
> know
> > when CPU 0 would read the true data. This may also be unacceptable
> > for us.
> >
> > Therefore, without memory barriers, in most time I believe everything
> > works fine. But to be more cautious, I still prefer to apply some
> > limitations to ensure blocked is always sync.
> >
> > Of course, probably my above understanding is incomplete or
> incorrect.
> > If so, please correct me.
> >
> >
> > Regards,
> > Ying
> >
> >
> > > Regards
> > > ///jon
> > >
> > >
> > >>
> > >> Signed-off-by: Ying Xue <yin...@wi...>
> > >> ---
> > >> net/tipc/bcast.c | 4 ++--
> > >> net/tipc/bearer.c | 33 ++++-----------------------------
> > >> net/tipc/bearer.h | 5 +----
> > >> net/tipc/discover.c | 5 +++--
> > >> net/tipc/eth_media.c | 9 ++++-----
> > >> net/tipc/ib_media.c | 9 ++++-----
> > >> net/tipc/link.c | 12 ++++++------
> > >> 7 files changed, 24 insertions(+), 53 deletions(-)
> > >>
> > >> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> > >> index 716de1a..c78f30c 100644
> > >> --- a/net/tipc/bcast.c
> > >> +++ b/net/tipc/bcast.c
> > >> @@ -615,8 +615,8 @@ static int tipc_bcbearer_send(struct sk_buff
> > *buf, struct tipc_bearer *unused1,
> > >> if (!p)
> > >> break; /* No more bearers to try */
> > >>
> > >> - if (tipc_bearer_blocked(p)) {
> > >> - if (!s || tipc_bearer_blocked(s))
> > >> + if (atomic_read(&p->blocked)) {
> > >> + if (!s || atomic_read(&s->blocked))
> > >> continue; /* Can't use either bearer
> */
> > >> b = s;
> > >> }
> > >> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> > >> index 3f9707a..17f9824 100644
> > >> --- a/net/tipc/bearer.c
> > >> +++ b/net/tipc/bearer.c
> > >> @@ -275,31 +275,6 @@ void tipc_bearer_remove_dest(struct
> > tipc_bearer *b_ptr, u32 dest)
> > >> tipc_disc_remove_dest(b_ptr->link_req);
> > >> }
> > >>
> > >> -/*
> > >> - * Interrupt enabling new requests after bearer blocking:
> > >> - * See bearer_send().
> > >> - */
> > >> -void tipc_continue(struct tipc_bearer *b)
> > >> -{
> > >> - spin_lock_bh(&b->lock);
> > >> - b->blocked = 0;
> > >> - spin_unlock_bh(&b->lock);
> > >> -}
> > >> -
> > >> -/*
> > >> - * tipc_bearer_blocked - determines if bearer is currently
> blocked
> > >> - */
> > >> -int tipc_bearer_blocked(struct tipc_bearer *b)
> > >> -{
> > >> - int res;
> > >> -
> > >> - spin_lock_bh(&b->lock);
> > >> - res = b->blocked;
> > >> - spin_unlock_bh(&b->lock);
> > >> -
> > >> - return res;
> > >> -}
> > >> -
> > >> /**
> > >> * tipc_enable_bearer - enable bearer with the given name
> > >> */
> > >> @@ -387,6 +362,8 @@ restart:
> > >>
> > >> b_ptr = &tipc_bearers[bearer_id];
> > >> strcpy(b_ptr->name, name);
> > >> + atomic_set(&b_ptr->blocked, 0);
> > >> + smp_mb();
> > >> res = m_ptr->enable_media(b_ptr);
> > >> if (res) {
> > >> pr_warn("Bearer <%s> rejected, enable failure
> (%d)\n",
> > >> @@ -429,8 +406,8 @@ int tipc_block_bearer(struct tipc_bearer
> *b_ptr)
> > >>
> > >> read_lock_bh(&tipc_net_lock);
> > >> pr_info("Blocking bearer <%s>\n", b_ptr->name);
> > >> + atomic_add_unless(&b_ptr->blocked, 1, 1);
> > >> spin_lock_bh(&b_ptr->lock);
> > >> - b_ptr->blocked = 1;
> > >> list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links,
> > link_list) {
> > >> struct tipc_node *n_ptr = l_ptr->owner;
> > >>
> > >> @@ -455,8 +432,8 @@ static void bearer_disable(struct tipc_bearer
> > *b_ptr)
> > >> struct tipc_link_req *temp_req;
> > >>
> >
> http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
> > _______________________________________________
> > tipc-discussion mailing list
> > tip...@li... <javascript:;>
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> >
> >
> >
> > --
> > Yours,
> > Jason
>
>
--
Yours,
Jason
|
|
From: Ying X. <yin...@wi...> - 2013-10-25 10:11:37
|
On 10/25/2013 03:48 PM, jason wrote:
> Hi Ying,
>
> On Friday, October 25, 2013, Ying Xue wrote:
>
> On 10/25/2013 11:20 AM, jason wrote:
> > Hi Ying,
> > As I under standing of the memory barrier, it is used for other CPU to
> > see a same order of two instructions (one before the memory
> barrier and
> > one after it) operating on memory as the local CPU. So if one
> decide to
> > use memory barrier, there must be at lease two memory operations
> first.
> > In your patch, operation on 'block' is just a single operation on
> memory
> > which just store a value to a memory location, no subsequent memory
> > operations presented to be execute in a strong order (must take effect
> > on memory AFTER set the 'block' flag) among all other CPUs. So I here
> > agree on Jon too, no need to use atomic, memory barrier and locks.
> >
>
> Please carefully read my mail before you post your comments.
>
> I have placed emphasis which there is no required data dependency in our
> case. But I have listed two exceptions: one is that some older
> processors have weak or even no cache coherences system. Specially you
> can look through implementations of memory barriers in all archs.
>
>
> Sorry for my careless...
>
> Yes, I got your point ,and you are correct absolutely. Yes, many
> architectures do need memory barrier to force to update cache, but in
> most case, you do not need to do it explicitly. Many kinds of implicit
> memory barrier will appear in the subsequent code, such as a
> rtnl_lock/rtnl_unlock... If you can find one, then your memory barrier
> operation can be omitted and improve performance.
>
>
> Another case is compiler optimization factor.
>
>
> Yes, memory barrier can suppress compiler optimization on the reader
> side. But I think as you know, a volatile key word is enough for this job.
>
Thanks for your confirmation.
Regards,
Ying
>
>
>
> //Ying
>
> > On Friday, October 25, 2013, Ying Xue wrote:
> >
> > On 10/21/2013 05:14 AM, Jon Maloy wrote:
> > > On 09/30/2013 10:38 AM, Ying Xue wrote:
> > >> The 'blocked' flag in struct tipc_bearer is currently
> protected by
> > >> a big spinlock aggregated into tipc_bearer. We want to
> reduce all
> > >> dependencies to this lock, as part of our effort to make
> the locking
> > >> policy simpler and more manageable.
> > >>
> > >> Furthermore, the functions tipc_disc_recv_msg() and
> disc_send_msg()
> > >> access the flag without taking this lock at all, implying a
> risk that
> > >> they may occasionally obtain a wrong value and drop
> sending/receiving
> > >> of discovery messages. Since discovery messages are timer
> driven and
> > >> best-effort, the latter problem is not fatal, but it is clearly
> > avoidable.
> > >>
> > >> By converting the type of the flag from int to atomic_t we
> eliminate
> > >> both the spinlock dependency and the current race condition.
> > >>
> > >> We also make two small code changes:
> > >>
> > >> 1) The functions tipc_continue() and bearer_blocked() now
> become
> > >> one-liners doing a single access to the flag. So we just
> remove
> > >> those redundant functions.
> > >>
> > >> 2) The initialization of the flag is moved from the
> enable_media()
> > >> functions in ib_media.c and eth_media.c respecively, to the
> > generic
> > >> tipc_enable_bearer() function, so that it is done in one
> place
> > only.
> > >
> > > Maybe it is just me, but I still fail to see the benefit (or
> harm)
> > of this change.
> > >
> > > As I understand it, the time line for the current
> implementation,
> > and the
> > > one you are suggesting, look like this:
> > >
> > > Without atomc_t:
> > > ------------------------
> > > 1: Device driver detects loss of carrier or similar.
> > > 2: It issues a call (work_queue?) to all registered users.
> > > 3: Until the call has reached the users, they (including TIPC)
> > will continue
> > > to send packets, although the driver can only drop them.
> > > 4: The field bearer->blocked is set as a one single write
> both on
> > 32-bits and
> > > 64-bist cPUs, since it is one word, and a small constant
> > value (1 or 0) that is written.
> > > 5: Other cores happening to work on the same cache line at
> that moment
> > > will notice nothing; they will continue to send the packet
> > they were working
> > > on, seeing the previous value of the field.
> > > 6: The cache line is flushed and reloaded as soon as next packet
> > is sent or received
> > > on these cores. (I don't think a single 64-byte cache line
> > will remain between
> > > two transmissions. There is way to many data fields
> involved.)
> > > 7: The next time the cache line is reloaded it has the correct
> > value of
> > > 'blocked', since the cache coherency HW support ensures that
> > > the cache is updated. Next packet is not sent.
> > >
> > >
> > > With atomic_t:
> > > ------------------
> > > 1: Device driver detects loss of carrier or similar.
> > > 2: It issues a call (work_queue?) to all registered users.
> > > 3: Until the call has reached the users, they (including TIPC)
> > will continue
> > > to send packets, although the driver can only drop them.
> > > 4: The field bearer->blocked is set as a single, atomic write,
> > which it would be
> > > anyway, just as in the previous case.
> > > 5: Other cores happening to read on the same cache line at
> that moment
> > > will now see the cache line invalidated, so they will
> have to
> > reread it with the
> > > new, correct value. The 'volatile' keyword ensures that.
> As I
> > understand
> > > GCC even puts in memory barrier around volatile
> variables, but
> > I fail to
> > > to see the relevance here, we don make any other changes
> to the
> > > bearer struct anyway, so ordereing is not an issue.
> > > 6: The cache line now has the correct read value. Packet is
> not sent.
> > >
> > > What have we won? That TIPC may have time to send one more
> > > packet per core than it would otherwise, but nothing can stop
> > > TIPC and others from sending a small number of packets
> during the
> > > interval between carrier loss detection and until the 'blocked'
> > flag is
> > > updated. The difference it makes if we cache line is invalidated
> > immediately
> > > or at next reload is microscopic in this context.
> > >
> > > Admittedly, the harm done is not big either. The memory
> > > barriers put in by GCC around the volatile read does not
> cost much.
> > > But why make such a change if we cannot motivate it?
> > >
> > > First of all, thank you for so detailed comments. In fact the
> > things you
> > concerned below often confuse me. So I spend some times trying to
> > understand memory cache system of modern processor as well as the
> > effects of kinds of Linux memory barrier primitives on memory
> cache
> > coherence. That's why I am late to respond you. But it's really so
> > complex that I am also afraid that I don't entirely understand
> them
> > although I consumed several days to research. Anyway, I am
> still going
> > to answer your questions :)
> >
> > Nowadays most of modern processors have cache coherence protocols
> > ensuring that all CPUs maintain a consistent view of data. The
> most
> > famous coherence protocol is called MESI. For example, the blocked
> > variable is manipulated by core 0 and is only written back to
> its local
> > cache, and other cores snoop/monitor the change of core 0 from
> internal
> > high speed bus and then invalidate their local cacheline where the
> > blocked variable resides. When other cores read the blocked,
> they can
> > get the up-to-date value from the cacheline of core 0. The
> whole process
> > is guaranteed by MESI. However, why do we still need memory
> barriers if
> > processor HW has cache coherence capability? This is because
> these CPUs
> > cannot ensure the order of accessing memory occurring on core
> 0 is same
> > as happening on other cores although they can guarantee that
> data is
> > consistent. But in our case, as there is no required data
> dependency,
> > memory barriers seem useless for us in this case.
> >
> > Unfortunately life doesn't sound like so easy! There are still
> some of
> > old CPUs being incapable of the cache coherence mechanisms,
> meaning that
> > manually flushing cache becomes mandatory for them. In this
> situation,
> > without memory barriers, the rate of getting stale data might
> > significantly arise. Even if many packets are involved(ie,
> many memory
> > areas would be access via CPU's local cache), in some
> extremely worst
> > case it doesn't mean the cacheline storing the stale blocked is
> > displaced/sacrificed within a short time. Therefore, it becomes
> > unpredictable for us when the stale blocked would be updated,
> which may
> > be unacceptable.
> >
> > If we find barrier.h files in kernel source tree, below
> results are got:
> >
> > */net-next-2.6$ find ./ -name barrier.h
> > ./arch/metag/include/asm/barrier.h
> > ./arch/sparc/include/asm/barrier.h
> > ./arch/h8300/include/asm/barrier.h
> > ./arch/x86/um/asm/barrier.h
> > ./arch/x86/include/asm/barrier.h
> > ./arch/unicore32/include/asm/barrier.h
> > ./arch/xtensa/include/asm/barrier.h
> > ./arch/frv/include/asm/barrier.h
> > ./arch/m68k/include/asm/barrier.h
> > ./arch/hexagon/include/asm/barrier.h
> > ./arch/mips/include/asm/barrier.h
> > ./arch/arm64/include/asm/barrier.h
> > ./arch/parisc/include/asm/barrier.h
> > ./arch/s390/include/asm/barrier.h
> > ./arch/ia64/include/asm/barrier.h
> > ./arch/m32r/include/asm/barrier.h
> > ./arch/powerpc/include/asm/barrier.h
> > ./arch/alpha/include/asm/barrier.h
> > ./arch/arm/include/asm/barrier.h
> > ./arch/avr32/include/asm/barrier.h
> > ./arch/sh/include/asm/barrier.h
> > ./arch/microblaze/include/asm/barrier.h
> > ./arch/mn10300/include/asm/barrier.h
> > ./arch/tile/include/asm/barrier.h
> > ./arch/arc/include/asm/barrier.h
> > ./arch/score/include/asm/barrier.h
> > ./arch/blackfin/include/asm/barrier.h
> > ./arch/cris/include/asm/barrier.h
> > ./include/asm-generic/barrier.h
> >
> > Different of architectures have different implementations.
> Even if arch
> > is same, different HW versions also have different
> implementations. If
> > we take a closer look at them, it's found many of implementations
> > forcedly flush/sync cache in memory barriers operations.
> >
> > Beside the effect on CPU cache, memory barriers also can
> suppress the
> > optimization of compiler. For it, I just attempt to state my
> > understanding of keyword - volatile, so please image below
> scenario:
> >
> > CPU 0 CPU 1
> > ----------- -----
> > T0: read blocked(blocked is 0 now)
> > T1: blocked = 1
> > T2: read blocked(blocked is 0)
> >
> > As compiler optimizes the process of accessing blocked,
> without volatile
> > keyword decorating the variable of blocked, at T0, CPU 0 reads
> blocked
> > from main memory/cache and load it into one of its registers.
> At T1,
> > blocked is set to 1 by CPU 1. But as CPU 0 is unaware of the
> change, it
> > gets the value from its register instead of main memory or
> cache when it
> > reads blocked variable again at T2. We don't know when the
> value in the
> > register will be overwritten by other values, that means we
> don't know
> > when CPU 0 would read the true data. This may also be unacceptable
> > for us.
> >
> > Therefore, without memory barriers, in most time I believe
> everything
> > works fine. But to be more cautious, I still prefer to apply some
> > limitations to ensure blocked is always sync.
> >
> > Of course, probably my above understanding is incomplete or
> incorrect.
> > If so, please correct me.
> >
> >
> > Regards,
> > Ying
> >
> >
> > > Regards
> > > ///jon
> > >
> > >
> > >>
> > >> Signed-off-by: Ying Xue <yin...@wi...>
> > >> ---
> > >> net/tipc/bcast.c | 4 ++--
> > >> net/tipc/bearer.c | 33 ++++-----------------------------
> > >> net/tipc/bearer.h | 5 +----
> > >> net/tipc/discover.c | 5 +++--
> > >> net/tipc/eth_media.c | 9 ++++-----
> > >> net/tipc/ib_media.c | 9 ++++-----
> > >> net/tipc/link.c | 12 ++++++------
> > >> 7 files changed, 24 insertions(+), 53 deletions(-)
> > >>
> > >> diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
> > >> index 716de1a..c78f30c 100644
> > >> --- a/net/tipc/bcast.c
> > >> +++ b/net/tipc/bcast.c
> > >> @@ -615,8 +615,8 @@ static int tipc_bcbearer_send(struct
> sk_buff
> > *buf, struct tipc_bearer *unused1,
> > >> if (!p)
> > >> break; /* No more bearers to try */
> > >>
> > >> - if (tipc_bearer_blocked(p)) {
> > >> - if (!s || tipc_bearer_blocked(s))
> > >> + if (atomic_read(&p->blocked)) {
> > >> + if (!s || atomic_read(&s->blocked))
> > >> continue; /* Can't use either
> bearer */
> > >> b = s;
> > >> }
> > >> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> > >> index 3f9707a..17f9824 100644
> > >> --- a/net/tipc/bearer.c
> > >> +++ b/net/tipc/bearer.c
> > >> @@ -275,31 +275,6 @@ void tipc_bearer_remove_dest(struct
> > tipc_bearer *b_ptr, u32 dest)
> > >> tipc_disc_remove_dest(b_ptr->link_req);
> > >> }
> > >>
> > >> -/*
> > >> - * Interrupt enabling new requests after bearer blocking:
> > >> - * See bearer_send().
> > >> - */
> > >> -void tipc_continue(struct tipc_bearer *b)
> > >> -{
> > >> - spin_lock_bh(&b->lock);
> > >> - b->blocked = 0;
> > >> - spin_unlock_bh(&b->lock);
> > >> -}
> > >> -
> > >> -/*
> > >> - * tipc_bearer_blocked - determines if bearer is currently
> blocked
> > >> - */
> > >> -int tipc_bearer_blocked(struct tipc_bearer *b)
> > >> -{
> > >> - int res;
> > >> -
> > >> - spin_lock_bh(&b->lock);
> > >> - res = b->blocked;
> > >> - spin_unlock_bh(&b->lock);
> > >> -
> > >> - return res;
> > >> -}
> > >> -
> > >> /**
> > >> * tipc_enable_bearer - enable bearer with the given name
> > >> */
> > >> @@ -387,6 +362,8 @@ restart:
> > >>
> > >> b_ptr = &tipc_bearers[bearer_id];
> > >> strcpy(b_ptr->name, name);
> > >> + atomic_set(&b_ptr->blocked, 0);
> > >> + smp_mb();
> > >> res = m_ptr->enable_media(b_ptr);
> > >> if (res) {
> > >> pr_warn("Bearer <%s> rejected, enable failure
> (%d)\n",
> > >> @@ -429,8 +406,8 @@ int tipc_block_bearer(struct
> tipc_bearer *b_ptr)
> > >>
> > >> read_lock_bh(&tipc_net_lock);
> > >> pr_info("Blocking bearer <%s>\n", b_ptr->name);
> > >> + atomic_add_unless(&b_ptr->blocked, 1, 1);
> > >> spin_lock_bh(&b_ptr->lock);
> > >> - b_ptr->blocked = 1;
> > >> list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links,
> > link_list) {
> > >> struct tipc_node *n_ptr = l_ptr->owner;
> > >>
> > >> @@ -455,8 +432,8 @@ static void bearer_disable(struct
> tipc_bearer
> > *b_ptr)
> > >> struct tipc_link_req *temp_req;
> > >>
> >
> http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk
> > _______________________________________________
> > tipc-discussion mailing list
> > tip...@li... <javascript:;>
> > https://lists.sourceforge.net/lists/listinfo/tipc-discussion
> >
> >
> >
> > --
> > Yours,
> > Jason
>
>
>
> --
> Yours,
> Jason
|
|
From: Erik H. <eri...@er...> - 2013-10-24 11:34:43
|
On Mon, Sep 30, 2013 at 04:38:13PM +0800, Ying Xue wrote: > The 'blocked' flag in struct tipc_bearer is currently protected by > a big spinlock aggregated into tipc_bearer. We want to reduce all > dependencies to this lock, as part of our effort to make the locking > policy simpler and more manageable. > > Furthermore, the functions tipc_disc_recv_msg() and disc_send_msg() > access the flag without taking this lock at all, implying a risk that > they may occasionally obtain a wrong value and drop sending/receiving > of discovery messages. Since discovery messages are timer driven and > best-effort, the latter problem is not fatal, but it is clearly avoidable. > > By converting the type of the flag from int to atomic_t we eliminate > both the spinlock dependency and the current race condition. > > We also make two small code changes: > > 1) The functions tipc_continue() and bearer_blocked() now become > one-liners doing a single access to the flag. So we just remove > those redundant functions. > > 2) The initialization of the flag is moved from the enable_media() > functions in ib_media.c and eth_media.c respecively, to the generic > tipc_enable_bearer() function, so that it is done in one place only. > > Signed-off-by: Ying Xue <yin...@wi...> Also, recent findings (off-list discussion) that we disobey the locking rules of del_timer_sync by holding the same spinlock in the timer handler as when we delete the timer, leading to deadlocks during bearer disable.. Acked-by: Erik Hugne <eri...@er...> |
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:38:46
|
When a TIPC media is started, it is registered to be linked into a
global media_list list, however, the media is not removed from the
list when it is stopped. But there has a special case that all TIPC
media instances are declared statically, so it cannot cause serious
problem like memory leak even if media instance is not removed from
the media_list list when TIPC module is unloaded.
As the media_list will be protected by RCU lock, without unregister
routine, we just see how a media instance is linked into media_list,
but cannot find where the media is removed from the list, which is
an very weird design. Therefore, to make TIPC code more readable the
tipc_unregister_media() is introduced to remove media instance from
media_list when TIPC module is stopped.
Signed-off-by: Ying Xue <yin...@wi...>
---
net/tipc/bearer.c | 17 +++++++++++++++++
net/tipc/bearer.h | 1 +
net/tipc/eth_media.c | 1 +
net/tipc/ib_media.c | 1 +
4 files changed, 20 insertions(+)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 17f9824..35e65b5 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -109,6 +109,23 @@ exit:
return res;
}
+void tipc_unregister_media(const struct tipc_media *m_ptr)
+{
+ struct tipc_media *m;
+ u32 i;
+
+ write_lock_bh(&tipc_net_lock);
+ for (i = 0; i < media_count; i++) {
+ m = media_list[i];
+ if (m_ptr == m) {
+ media_list[i] = NULL;
+ media_count--;
+ break;
+ }
+ }
+ write_unlock_bh(&tipc_net_lock);
+}
+
/**
* tipc_media_addr_printf - record media address in print buffer
*/
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 8695d4a..280c23d 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -160,6 +160,7 @@ extern struct tipc_bearer tipc_bearers[];
* TIPC routines available to supported media types
*/
int tipc_register_media(struct tipc_media *m_ptr);
+void tipc_unregister_media(const struct tipc_media *m_ptr);
void tipc_recv_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr);
diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index 32d01e8..5ba58b0 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -371,5 +371,6 @@ void tipc_eth_media_stop(void)
flush_scheduled_work();
unregister_netdevice_notifier(¬ifier);
+ tipc_unregister_media(ð_media_info);
eth_started = 0;
}
diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
index 5241a5b..34b9710 100644
--- a/net/tipc/ib_media.c
+++ b/net/tipc/ib_media.c
@@ -367,5 +367,6 @@ void tipc_ib_media_stop(void)
flush_scheduled_work();
unregister_netdevice_notifier(¬ifier);
+ tipc_unregister_media(&ib_media_info);
ib_started = 0;
}
--
1.7.9.5
|
|
From: Jon M. <ma...@do...> - 2013-10-20 20:29:36
|
On 09/30/2013 10:38 AM, Ying Xue wrote:
> When a TIPC media is started, it is registered to be linked into a
> global media_list list, however, the media is not removed from the
> list when it is stopped. But there has a special case that all TIPC
> media instances are declared statically, so it cannot cause serious
> problem like memory leak even if media instance is not removed from
> the media_list list when TIPC module is unloaded.
>
> As the media_list will be protected by RCU lock, without unregister
> routine, we just see how a media instance is linked into media_list,
> but cannot find where the media is removed from the list, which is
> an very weird design. Therefore, to make TIPC code more readable the
> tipc_unregister_media() is introduced to remove media instance from
> media_list when TIPC module is stopped.
See previous comment about getting rid of this list and aggregate
tipc_media into eth/ib_bearer.
If you disagree with this, we should as a minimum rename it to
media[] or media_array[], since the term list is very misleading.
///jon
>
> Signed-off-by: Ying Xue <yin...@wi...>
> ---
> net/tipc/bearer.c | 17 +++++++++++++++++
> net/tipc/bearer.h | 1 +
> net/tipc/eth_media.c | 1 +
> net/tipc/ib_media.c | 1 +
> 4 files changed, 20 insertions(+)
>
> diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
> index 17f9824..35e65b5 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -109,6 +109,23 @@ exit:
> return res;
> }
>
> +void tipc_unregister_media(const struct tipc_media *m_ptr)
> +{
> + struct tipc_media *m;
> + u32 i;
> +
> + write_lock_bh(&tipc_net_lock);
> + for (i = 0; i < media_count; i++) {
> + m = media_list[i];
> + if (m_ptr == m) {
> + media_list[i] = NULL;
> + media_count--;
> + break;
> + }
> + }
> + write_unlock_bh(&tipc_net_lock);
> +}
> +
> /**
> * tipc_media_addr_printf - record media address in print buffer
> */
> diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
> index 8695d4a..280c23d 100644
> --- a/net/tipc/bearer.h
> +++ b/net/tipc/bearer.h
> @@ -160,6 +160,7 @@ extern struct tipc_bearer tipc_bearers[];
> * TIPC routines available to supported media types
> */
> int tipc_register_media(struct tipc_media *m_ptr);
> +void tipc_unregister_media(const struct tipc_media *m_ptr);
>
> void tipc_recv_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr);
>
> diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
> index 32d01e8..5ba58b0 100644
> --- a/net/tipc/eth_media.c
> +++ b/net/tipc/eth_media.c
> @@ -371,5 +371,6 @@ void tipc_eth_media_stop(void)
>
> flush_scheduled_work();
> unregister_netdevice_notifier(¬ifier);
> + tipc_unregister_media(ð_media_info);
> eth_started = 0;
> }
> diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
> index 5241a5b..34b9710 100644
> --- a/net/tipc/ib_media.c
> +++ b/net/tipc/ib_media.c
> @@ -367,5 +367,6 @@ void tipc_ib_media_stop(void)
>
> flush_scheduled_work();
> unregister_netdevice_notifier(¬ifier);
> + tipc_unregister_media(&ib_media_info);
> ib_started = 0;
> }
|
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:38:50
|
When TIPC media is enabled by bearer, the "bearer" variable in the
media instance is first initialized with the bearer pointer under
tipc_net_lock protection; likewise, when media is disabled,
tipc_net_lock is also first held and its "bearer" variable is then
set to NULL. But, when the "bearer" variable is accessed in
recv_msg() and recv_notification(), tipc_net_lock is not taken. As
the context under which recv_msg() and recv_notification() run is
different with media enable/disable functions, a stale "bearer"
might be got by recv_msg() and recv_notification() occasionally.
Therefore, before the bearer variable is read, tipc_net_lock should
be held to get rid of the synchronous issue.
Signed-off-by: Ying Xue <yin...@wi...>
---
net/tipc/bearer.c | 2 --
net/tipc/eth_media.c | 13 ++++++++++++-
net/tipc/ib_media.c | 13 ++++++++++++-
net/tipc/link.c | 2 --
4 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 35e65b5..9f03a93 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -421,7 +421,6 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
struct tipc_link *l_ptr;
struct tipc_link *temp_l_ptr;
- read_lock_bh(&tipc_net_lock);
pr_info("Blocking bearer <%s>\n", b_ptr->name);
atomic_add_unless(&b_ptr->blocked, 1, 1);
spin_lock_bh(&b_ptr->lock);
@@ -433,7 +432,6 @@ int tipc_block_bearer(struct tipc_bearer *b_ptr)
spin_unlock_bh(&n_ptr->lock);
}
spin_unlock_bh(&b_ptr->lock);
- read_unlock_bh(&tipc_net_lock);
return 0;
}
diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index 5ba58b0..f7ddbd7 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -36,6 +36,7 @@
#include "core.h"
#include "bearer.h"
+#include "net.h"
#define MAX_ETH_MEDIA MAX_BEARERS
@@ -135,13 +136,16 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
return NET_RX_DROP;
}
+ read_lock_bh(&tipc_net_lock);
if (likely(eb_ptr->bearer)) {
if (likely(buf->pkt_type <= PACKET_BROADCAST)) {
buf->next = NULL;
tipc_recv_msg(buf, eb_ptr->bearer);
+ read_unlock_bh(&tipc_net_lock);
return NET_RX_SUCCESS;
}
}
+ read_unlock_bh(&tipc_net_lock);
kfree_skb(buf);
return NET_RX_DROP;
}
@@ -254,8 +258,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
if (++eb_ptr == stop)
return NOTIFY_DONE; /* couldn't find device */
}
- if (!eb_ptr->bearer)
+
+ read_lock_bh(&tipc_net_lock);
+ if (!eb_ptr->bearer) {
+ read_unlock_bh(&tipc_net_lock);
return NOTIFY_DONE; /* bearer had been disabled */
+ }
eb_ptr->bearer->mtu = dev->mtu;
@@ -279,9 +287,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
break;
case NETDEV_UNREGISTER:
case NETDEV_CHANGENAME:
+ read_unlock_bh(&tipc_net_lock);
tipc_disable_bearer(eb_ptr->bearer->name);
+ read_lock_bh(&tipc_net_lock);
break;
}
+ read_unlock_bh(&tipc_net_lock);
return NOTIFY_OK;
}
diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
index 34b9710..1d1e2f5 100644
--- a/net/tipc/ib_media.c
+++ b/net/tipc/ib_media.c
@@ -41,6 +41,7 @@
#include <linux/if_infiniband.h>
#include "core.h"
#include "bearer.h"
+#include "net.h"
#define MAX_IB_MEDIA MAX_BEARERS
@@ -128,13 +129,16 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
return NET_RX_DROP;
}
+ read_lock_bh(&tipc_net_lock);
if (likely(ib_ptr->bearer)) {
if (likely(buf->pkt_type <= PACKET_BROADCAST)) {
buf->next = NULL;
tipc_recv_msg(buf, ib_ptr->bearer);
+ read_unlock_bh(&tipc_net_lock);
return NET_RX_SUCCESS;
}
}
+ read_unlock_bh(&tipc_net_lock);
kfree_skb(buf);
return NET_RX_DROP;
}
@@ -247,8 +251,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
if (++ib_ptr == stop)
return NOTIFY_DONE; /* couldn't find device */
}
- if (!ib_ptr->bearer)
+
+ read_lock_bh(&tipc_net_lock);
+ if (!ib_ptr->bearer) {
+ read_unlock_bh(&tipc_net_lock);
return NOTIFY_DONE; /* bearer had been disabled */
+ }
ib_ptr->bearer->mtu = dev->mtu;
@@ -272,9 +280,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
break;
case NETDEV_UNREGISTER:
case NETDEV_CHANGENAME:
+ read_unlock_bh(&tipc_net_lock);
tipc_disable_bearer(ib_ptr->bearer->name);
+ read_lock_bh(&tipc_net_lock);
break;
}
+ read_unlock_bh(&tipc_net_lock);
return NOTIFY_OK;
}
diff --git a/net/tipc/link.c b/net/tipc/link.c
index 8c0e9a3..1069874 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1491,7 +1491,6 @@ static int link_recv_buf_validate(struct sk_buff *buf)
*/
void tipc_recv_msg(struct sk_buff *head, struct tipc_bearer *b_ptr)
{
- read_lock_bh(&tipc_net_lock);
while (head) {
struct tipc_node *n_ptr;
struct tipc_link *l_ptr;
@@ -1687,7 +1686,6 @@ deliver:
cont:
kfree_skb(buf);
}
- read_unlock_bh(&tipc_net_lock);
}
/**
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:38:53
|
When TIPC protocol handler is registered into networking stack by TIPC media, the media pointer is also attched to af_packet_priv of the protocol handler; when TIPC packets arrive at recv_msg(), media pointer can be got from af_packet_priv in TIPC protocol handler which is passed as one of arguments of recv_msg(). With the media pointer, packets can be injected into TIPC stack. Although this registration now works well for TIPC, the usage of af_packet_priv is totally wrong because af_packet_priv should defined only for AF_PACKET socket. About its more detailed discussion, please see below link: http://patchwork.ozlabs.org/patch/178044/ Therefore, in order to git rid of af_packet_priv from TIPC stack when TIPC protocol handler is registered, the dynamically allocated object of eth_media/ib_media is inserted into one hash list with one hash key to which the index of network device associated with the media is used; when TIPC packets reach recv_msg() from networking device, the index of the network device is used as hash key to find corresponding media object from the hash list. Signed-off-by: Ying Xue <yin...@wi...> --- net/tipc/eth_media.c | 116 ++++++++++++++++++++++++++++---------------------- net/tipc/ib_media.c | 103 ++++++++++++++++++++++++++------------------ 2 files changed, 126 insertions(+), 93 deletions(-) diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c index f7ddbd7..64e3f1c 100644 --- a/net/tipc/eth_media.c +++ b/net/tipc/eth_media.c @@ -38,39 +38,44 @@ #include "bearer.h" #include "net.h" -#define MAX_ETH_MEDIA MAX_BEARERS - #define ETH_ADDR_OFFSET 4 /* message header offset of MAC address */ /** * struct eth_media - Ethernet bearer data structure * @bearer: ptr to associated "generic" bearer structure + * @hlist: Ethernet media hash chain * @dev: ptr to associated Ethernet network device - * @tipc_packet_type: used in binding TIPC to Ethernet driver * @setup: work item used when enabling bearer * @cleanup: work item used when disabling bearer */ struct eth_media { struct tipc_bearer *bearer; + struct hlist_node hlist; struct net_device *dev; - struct packet_type tipc_packet_type; struct work_struct setup; struct work_struct cleanup; }; static struct tipc_media eth_media_info; -static struct eth_media eth_media_array[MAX_ETH_MEDIA]; +static struct hlist_head *media_hlist; static int eth_started; +static struct packet_type tipc_packet_type __read_mostly; -static int recv_notification(struct notifier_block *nb, unsigned long evt, - void *dv); -/* - * Network device notifier info - */ -static struct notifier_block notifier = { - .notifier_call = recv_notification, - .priority = 0 -}; +static inline struct hlist_head *media_hashfn(int ifindex) +{ + return &media_hlist[ifindex & (NETDEV_HASHENTRIES - 1)]; +} + +static struct eth_media *media_get(int ifindex) +{ + struct eth_media *eb_ptr; + struct hlist_head *head = media_hashfn(ifindex); + + hlist_for_each_entry(eb_ptr, head, hlist) + if (eb_ptr->dev->ifindex == ifindex) + return eb_ptr; + return NULL; +} /** * eth_media_addr_set - initialize Ethernet media address structure @@ -129,7 +134,7 @@ static int send_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr, static int recv_msg(struct sk_buff *buf, struct net_device *dev, struct packet_type *pt, struct net_device *orig_dev) { - struct eth_media *eb_ptr = (struct eth_media *)pt->af_packet_priv; + struct eth_media *eb_ptr; if (!net_eq(dev_net(dev), &init_net)) { kfree_skb(buf); @@ -137,7 +142,8 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev, } read_lock_bh(&tipc_net_lock); - if (likely(eb_ptr->bearer)) { + eb_ptr = media_get(dev->ifindex); + if (likely(eb_ptr)) { if (likely(buf->pkt_type <= PACKET_BROADCAST)) { buf->next = NULL; tipc_recv_msg(buf, eb_ptr->bearer); @@ -155,10 +161,7 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev, */ static void setup_media(struct work_struct *work) { - struct eth_media *eb_ptr = - container_of(work, struct eth_media, setup); - - dev_add_pack(&eb_ptr->tipc_packet_type); + dev_add_pack(&tipc_packet_type); } /** @@ -167,18 +170,8 @@ static void setup_media(struct work_struct *work) static int enable_media(struct tipc_bearer *tb_ptr) { struct net_device *dev; - struct eth_media *eb_ptr = ð_media_array[0]; - struct eth_media *stop = ð_media_array[MAX_ETH_MEDIA]; + struct eth_media *eb_ptr; char *driver_name = strchr((const char *)tb_ptr->name, ':') + 1; - int pending_dev = 0; - - /* Find unused Ethernet bearer structure */ - while (eb_ptr->dev) { - if (!eb_ptr->bearer) - pending_dev++; - if (++eb_ptr == stop) - return pending_dev ? -EAGAIN : -EDQUOT; - } /* Find device with specified name */ dev = dev_get_by_name(&init_net, driver_name); @@ -186,12 +179,11 @@ static int enable_media(struct tipc_bearer *tb_ptr) return -ENODEV; /* Create Ethernet bearer for device */ + eb_ptr = kzalloc(sizeof(*eb_ptr), GFP_ATOMIC); + if (!eb_ptr) + return -ENOMEM; + eb_ptr->dev = dev; - eb_ptr->tipc_packet_type.type = htons(ETH_P_TIPC); - eb_ptr->tipc_packet_type.dev = dev; - eb_ptr->tipc_packet_type.func = recv_msg; - eb_ptr->tipc_packet_type.af_packet_priv = eb_ptr; - INIT_LIST_HEAD(&(eb_ptr->tipc_packet_type.list)); INIT_WORK(&eb_ptr->setup, setup_media); schedule_work(&eb_ptr->setup); @@ -204,6 +196,7 @@ static int enable_media(struct tipc_bearer *tb_ptr) tb_ptr->bcast_addr.broadcast = 1; tb_ptr->mtu = dev->mtu; eth_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr); + hlist_add_head(&eb_ptr->hlist, media_hashfn(dev->ifindex)); return 0; } @@ -217,9 +210,9 @@ static void cleanup_media(struct work_struct *work) struct eth_media *eb_ptr = container_of(work, struct eth_media, cleanup); - dev_remove_pack(&eb_ptr->tipc_packet_type); + dev_remove_pack(&tipc_packet_type); dev_put(eb_ptr->dev); - eb_ptr->dev = NULL; + kfree(eb_ptr); } /** @@ -233,7 +226,7 @@ static void disable_media(struct tipc_bearer *tb_ptr) { struct eth_media *eb_ptr = (struct eth_media *)tb_ptr->usr_handle; - eb_ptr->bearer = NULL; + hlist_del(&eb_ptr->hlist); INIT_WORK(&eb_ptr->cleanup, cleanup_media); schedule_work(&eb_ptr->cleanup); } @@ -248,19 +241,14 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, void *ptr) { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - struct eth_media *eb_ptr = ð_media_array[0]; - struct eth_media *stop = ð_media_array[MAX_ETH_MEDIA]; + struct eth_media *eb_ptr; if (!net_eq(dev_net(dev), &init_net)) return NOTIFY_DONE; - while ((eb_ptr->dev != dev)) { - if (++eb_ptr == stop) - return NOTIFY_DONE; /* couldn't find device */ - } - read_lock_bh(&tipc_net_lock); - if (!eb_ptr->bearer) { + eb_ptr = media_get(dev->ifindex); + if (!eb_ptr) { read_unlock_bh(&tipc_net_lock); return NOTIFY_DONE; /* bearer had been disabled */ } @@ -349,6 +337,16 @@ static struct tipc_media eth_media_info = { .name = "eth" }; +static struct notifier_block notifier = { + .notifier_call = recv_notification, + .priority = 0 +}; + +static struct packet_type tipc_packet_type __read_mostly = { + .type = __constant_htons(ETH_P_TIPC), + .func = recv_msg, +}; + /** * tipc_eth_media_start - activate Ethernet bearer support * @@ -357,18 +355,33 @@ static struct tipc_media eth_media_info = { */ int tipc_eth_media_start(void) { - int res; + int res, i; if (eth_started) return -EINVAL; + media_hlist = kmalloc(sizeof(*media_hlist) * NETDEV_HASHENTRIES, + GFP_KERNEL); + if (!media_hlist) + return -ENOMEM; + + for (i = 0; i < NETDEV_HASHENTRIES; i++) + INIT_HLIST_HEAD(&media_hlist[i]); + res = tipc_register_media(ð_media_info); - if (res) + if (res) { + kfree(media_hlist); return res; + } res = register_netdevice_notifier(¬ifier); - if (!res) - eth_started = 1; + if (res) { + kfree(media_hlist); + tipc_unregister_media(ð_media_info); + return res; + } + + eth_started = 1; return res; } @@ -383,5 +396,6 @@ void tipc_eth_media_stop(void) flush_scheduled_work(); unregister_netdevice_notifier(¬ifier); tipc_unregister_media(ð_media_info); + kfree(media_hlist); eth_started = 0; } diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c index 1d1e2f5..d37c9c5 100644 --- a/net/tipc/ib_media.c +++ b/net/tipc/ib_media.c @@ -43,27 +43,42 @@ #include "bearer.h" #include "net.h" -#define MAX_IB_MEDIA MAX_BEARERS - /** * struct ib_media - Infiniband media data structure * @bearer: ptr to associated "generic" bearer structure + * @hlist: Infiniband media hash chain * @dev: ptr to associated Infiniband network device - * @tipc_packet_type: used in binding TIPC to Infiniband driver * @cleanup: work item used when disabling bearer */ struct ib_media { struct tipc_bearer *bearer; + struct hlist_node hlist; struct net_device *dev; - struct packet_type tipc_packet_type; struct work_struct setup; struct work_struct cleanup; }; static struct tipc_media ib_media_info; -static struct ib_media ib_media_array[MAX_IB_MEDIA]; +static struct hlist_head *media_hlist; static int ib_started; +static struct packet_type tipc_packet_type __read_mostly; + +static inline struct hlist_head *media_hashfn(int ifindex) +{ + return &media_hlist[ifindex & (NETDEV_HASHENTRIES - 1)]; +} + +static struct ib_media *media_get(int ifindex) +{ + struct ib_media *ib_ptr; + struct hlist_head *head = media_hashfn(ifindex); + + hlist_for_each_entry(ib_ptr, head, hlist) + if (ib_ptr->dev->ifindex == ifindex) + return ib_ptr; + return NULL; +} /** * ib_media_addr_set - initialize Infiniband media address structure @@ -122,7 +137,7 @@ static int send_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr, static int recv_msg(struct sk_buff *buf, struct net_device *dev, struct packet_type *pt, struct net_device *orig_dev) { - struct ib_media *ib_ptr = (struct ib_media *)pt->af_packet_priv; + struct ib_media *ib_ptr; if (!net_eq(dev_net(dev), &init_net)) { kfree_skb(buf); @@ -130,7 +145,8 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev, } read_lock_bh(&tipc_net_lock); - if (likely(ib_ptr->bearer)) { + ib_ptr = media_get(dev->ifindex); + if (likely(ib_ptr)) { if (likely(buf->pkt_type <= PACKET_BROADCAST)) { buf->next = NULL; tipc_recv_msg(buf, ib_ptr->bearer); @@ -148,10 +164,7 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev, */ static void setup_media(struct work_struct *work) { - struct ib_media *ib_ptr = - container_of(work, struct ib_media, setup); - - dev_add_pack(&ib_ptr->tipc_packet_type); + dev_add_pack(&tipc_packet_type); } /** @@ -160,18 +173,8 @@ static void setup_media(struct work_struct *work) static int enable_media(struct tipc_bearer *tb_ptr) { struct net_device *dev; - struct ib_media *ib_ptr = &ib_media_array[0]; - struct ib_media *stop = &ib_media_array[MAX_IB_MEDIA]; + struct ib_media *ib_ptr; char *driver_name = strchr((const char *)tb_ptr->name, ':') + 1; - int pending_dev = 0; - - /* Find unused InfiniBand bearer structure */ - while (ib_ptr->dev) { - if (!ib_ptr->bearer) - pending_dev++; - if (++ib_ptr == stop) - return pending_dev ? -EAGAIN : -EDQUOT; - } /* Find device with specified name */ dev = dev_get_by_name(&init_net, driver_name); @@ -179,12 +182,11 @@ static int enable_media(struct tipc_bearer *tb_ptr) return -ENODEV; /* Create InfiniBand bearer for device */ + ib_ptr = kzalloc(sizeof(*ib_ptr), GFP_ATOMIC); + if (!ib_ptr) + return -ENOMEM; + ib_ptr->dev = dev; - ib_ptr->tipc_packet_type.type = htons(ETH_P_TIPC); - ib_ptr->tipc_packet_type.dev = dev; - ib_ptr->tipc_packet_type.func = recv_msg; - ib_ptr->tipc_packet_type.af_packet_priv = ib_ptr; - INIT_LIST_HEAD(&(ib_ptr->tipc_packet_type.list)); INIT_WORK(&ib_ptr->setup, setup_media); schedule_work(&ib_ptr->setup); @@ -197,6 +199,7 @@ static int enable_media(struct tipc_bearer *tb_ptr) tb_ptr->bcast_addr.broadcast = 1; tb_ptr->mtu = dev->mtu; ib_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr); + hlist_add_head(&ib_ptr->hlist, media_hashfn(dev->ifindex)); return 0; } @@ -210,9 +213,9 @@ static void cleanup_bearer(struct work_struct *work) struct ib_media *ib_ptr = container_of(work, struct ib_media, cleanup); - dev_remove_pack(&ib_ptr->tipc_packet_type); + dev_remove_pack(&tipc_packet_type); dev_put(ib_ptr->dev); - ib_ptr->dev = NULL; + kfree(ib_ptr); } /** @@ -226,7 +229,7 @@ static void disable_media(struct tipc_bearer *tb_ptr) { struct ib_media *ib_ptr = (struct ib_media *)tb_ptr->usr_handle; - ib_ptr->bearer = NULL; + hlist_del(&ib_ptr->hlist); INIT_WORK(&ib_ptr->cleanup, cleanup_bearer); schedule_work(&ib_ptr->cleanup); } @@ -241,19 +244,14 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt, void *ptr) { struct net_device *dev = netdev_notifier_info_to_dev(ptr); - struct ib_media *ib_ptr = &ib_media_array[0]; - struct ib_media *stop = &ib_media_array[MAX_IB_MEDIA]; + struct ib_media *ib_ptr; if (!net_eq(dev_net(dev), &init_net)) return NOTIFY_DONE; - while ((ib_ptr->dev != dev)) { - if (++ib_ptr == stop) - return NOTIFY_DONE; /* couldn't find device */ - } - read_lock_bh(&tipc_net_lock); - if (!ib_ptr->bearer) { + ib_ptr = media_get(dev->ifindex); + if (!ib_ptr) { read_unlock_bh(&tipc_net_lock); return NOTIFY_DONE; /* bearer had been disabled */ } @@ -294,6 +292,11 @@ static struct notifier_block notifier = { .priority = 0, }; +static struct packet_type tipc_packet_type __read_mostly = { + .type = __constant_htons(ETH_P_TIPC), + .func = recv_msg, +}; + /** * ib_addr2str - convert InfiniBand address to string */ @@ -353,18 +356,33 @@ static struct tipc_media ib_media_info = { */ int tipc_ib_media_start(void) { - int res; + int res, i; if (ib_started) return -EINVAL; + media_hlist = kmalloc(sizeof(*media_hlist) * NETDEV_HASHENTRIES, + GFP_KERNEL); + if (!media_hlist) + return -ENOMEM; + + for (i = 0; i < NETDEV_HASHENTRIES; i++) + INIT_HLIST_HEAD(&media_hlist[i]); + res = tipc_register_media(&ib_media_info); - if (res) + if (res) { + kfree(media_hlist); return res; + } res = register_netdevice_notifier(¬ifier); - if (!res) - ib_started = 1; + if (res) { + kfree(media_hlist); + tipc_unregister_media(&ib_media_info); + return res; + } + + ib_started = 1; return res; } @@ -379,5 +397,6 @@ void tipc_ib_media_stop(void) flush_scheduled_work(); unregister_netdevice_notifier(¬ifier); tipc_unregister_media(&ib_media_info); + kfree(media_hlist); ib_started = 0; } -- 1.7.9.5 |
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:38:57
|
Since setup/cleanup work items are eliminated from eth_media/ib_media structure, it's meaningless to call flush_scheduled_work() in media stop routines. Signed-off-by: Ying Xue <yin...@wi...> --- net/tipc/eth_media.c | 1 - net/tipc/ib_media.c | 1 - 2 files changed, 2 deletions(-) diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c index e8870e8..0480299 100644 --- a/net/tipc/eth_media.c +++ b/net/tipc/eth_media.c @@ -363,7 +363,6 @@ void tipc_eth_media_stop(void) if (!eth_started) return; - flush_scheduled_work(); unregister_netdevice_notifier(¬ifier); dev_remove_pack(&tipc_packet_type); tipc_unregister_media(ð_media_info); diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c index c9dbf3b..916f60e 100644 --- a/net/tipc/ib_media.c +++ b/net/tipc/ib_media.c @@ -365,7 +365,6 @@ void tipc_ib_media_stop(void) if (!ib_started) return; - flush_scheduled_work(); unregister_netdevice_notifier(¬ifier); dev_remove_pack(&tipc_packet_type); tipc_unregister_media(&ib_media_info); -- 1.7.9.5 |
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:39:04
|
When TIPC packet handler is registered into networking stack, it's
enough to register once for all same type of media. So we should move
the registration/unregistration of TIPC packet handler to media start
and media stop routines.
In doing so, we can also delete all the work_struct related setup and
cleanup infrastructures because media start/stop routines are now
executed under process context.
Signed-off-by: Ying Xue <yin...@wi...>
---
net/tipc/eth_media.c | 41 ++++++-----------------------------------
net/tipc/ib_media.c | 40 ++++++----------------------------------
2 files changed, 12 insertions(+), 69 deletions(-)
diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index 64e3f1c..e8870e8 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -45,21 +45,16 @@
* @bearer: ptr to associated "generic" bearer structure
* @hlist: Ethernet media hash chain
* @dev: ptr to associated Ethernet network device
- * @setup: work item used when enabling bearer
- * @cleanup: work item used when disabling bearer
*/
struct eth_media {
struct tipc_bearer *bearer;
struct hlist_node hlist;
struct net_device *dev;
- struct work_struct setup;
- struct work_struct cleanup;
};
static struct tipc_media eth_media_info;
static struct hlist_head *media_hlist;
static int eth_started;
-static struct packet_type tipc_packet_type __read_mostly;
static inline struct hlist_head *media_hashfn(int ifindex)
{
@@ -157,14 +152,6 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
}
/**
- * setup_media - setup association between Ethernet bearer and interface
- */
-static void setup_media(struct work_struct *work)
-{
- dev_add_pack(&tipc_packet_type);
-}
-
-/**
* enable_media - attach TIPC bearer to an Ethernet interface
*/
static int enable_media(struct tipc_bearer *tb_ptr)
@@ -184,8 +171,6 @@ static int enable_media(struct tipc_bearer *tb_ptr)
return -ENOMEM;
eb_ptr->dev = dev;
- INIT_WORK(&eb_ptr->setup, setup_media);
- schedule_work(&eb_ptr->setup);
/* Associate TIPC bearer with Ethernet bearer */
eb_ptr->bearer = tb_ptr;
@@ -201,34 +186,17 @@ static int enable_media(struct tipc_bearer *tb_ptr)
}
/**
- * cleanup_media - break association between Ethernet bearer and interface
- *
- * This routine must be invoked from a work queue because it can sleep.
- */
-static void cleanup_media(struct work_struct *work)
-{
- struct eth_media *eb_ptr =
- container_of(work, struct eth_media, cleanup);
-
- dev_remove_pack(&tipc_packet_type);
- dev_put(eb_ptr->dev);
- kfree(eb_ptr);
-}
-
-/**
* disable_media - detach TIPC bearer from an Ethernet interface
*
- * Mark Ethernet bearer as inactive so that incoming buffers are thrown away,
- * then get worker thread to complete bearer cleanup. (Can't do cleanup
- * here because cleanup code needs to sleep and caller holds spinlocks.)
+ * Mark Ethernet bearer as inactive so that incoming buffers are thrown away.
*/
static void disable_media(struct tipc_bearer *tb_ptr)
{
struct eth_media *eb_ptr = (struct eth_media *)tb_ptr->usr_handle;
hlist_del(&eb_ptr->hlist);
- INIT_WORK(&eb_ptr->cleanup, cleanup_media);
- schedule_work(&eb_ptr->cleanup);
+ dev_put(eb_ptr->dev);
+ kfree(eb_ptr);
}
/**
@@ -381,6 +349,8 @@ int tipc_eth_media_start(void)
return res;
}
+ dev_add_pack(&tipc_packet_type);
+
eth_started = 1;
return res;
}
@@ -395,6 +365,7 @@ void tipc_eth_media_stop(void)
flush_scheduled_work();
unregister_netdevice_notifier(¬ifier);
+ dev_remove_pack(&tipc_packet_type);
tipc_unregister_media(ð_media_info);
kfree(media_hlist);
eth_started = 0;
diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
index d37c9c5..c9dbf3b 100644
--- a/net/tipc/ib_media.c
+++ b/net/tipc/ib_media.c
@@ -48,21 +48,17 @@
* @bearer: ptr to associated "generic" bearer structure
* @hlist: Infiniband media hash chain
* @dev: ptr to associated Infiniband network device
- * @cleanup: work item used when disabling bearer
*/
struct ib_media {
struct tipc_bearer *bearer;
struct hlist_node hlist;
struct net_device *dev;
- struct work_struct setup;
- struct work_struct cleanup;
};
static struct tipc_media ib_media_info;
static struct hlist_head *media_hlist;
static int ib_started;
-static struct packet_type tipc_packet_type __read_mostly;
static inline struct hlist_head *media_hashfn(int ifindex)
{
@@ -160,14 +156,6 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
}
/**
- * setup_bearer - setup association between InfiniBand bearer and interface
- */
-static void setup_media(struct work_struct *work)
-{
- dev_add_pack(&tipc_packet_type);
-}
-
-/**
* enable_media - attach TIPC bearer to an InfiniBand interface
*/
static int enable_media(struct tipc_bearer *tb_ptr)
@@ -187,8 +175,6 @@ static int enable_media(struct tipc_bearer *tb_ptr)
return -ENOMEM;
ib_ptr->dev = dev;
- INIT_WORK(&ib_ptr->setup, setup_media);
- schedule_work(&ib_ptr->setup);
/* Associate TIPC bearer with InfiniBand bearer */
ib_ptr->bearer = tb_ptr;
@@ -204,34 +190,17 @@ static int enable_media(struct tipc_bearer *tb_ptr)
}
/**
- * cleanup_bearer - break association between InfiniBand bearer and interface
- *
- * This routine must be invoked from a work queue because it can sleep.
- */
-static void cleanup_bearer(struct work_struct *work)
-{
- struct ib_media *ib_ptr =
- container_of(work, struct ib_media, cleanup);
-
- dev_remove_pack(&tipc_packet_type);
- dev_put(ib_ptr->dev);
- kfree(ib_ptr);
-}
-
-/**
* disable_media - detach TIPC bearer from an InfiniBand interface
*
- * Mark InfiniBand bearer as inactive so that incoming buffers are thrown away,
- * then get worker thread to complete bearer cleanup. (Can't do cleanup
- * here because cleanup code needs to sleep and caller holds spinlocks.)
+ * Mark InfiniBand bearer as inactive so that incoming buffers are thrown away.
*/
static void disable_media(struct tipc_bearer *tb_ptr)
{
struct ib_media *ib_ptr = (struct ib_media *)tb_ptr->usr_handle;
hlist_del(&ib_ptr->hlist);
- INIT_WORK(&ib_ptr->cleanup, cleanup_bearer);
- schedule_work(&ib_ptr->cleanup);
+ dev_put(ib_ptr->dev);
+ kfree(ib_ptr);
}
/**
@@ -382,6 +351,8 @@ int tipc_ib_media_start(void)
return res;
}
+ dev_add_pack(&tipc_packet_type);
+
ib_started = 1;
return res;
}
@@ -396,6 +367,7 @@ void tipc_ib_media_stop(void)
flush_scheduled_work();
unregister_netdevice_notifier(¬ifier);
+ dev_remove_pack(&tipc_packet_type);
tipc_unregister_media(&ib_media_info);
kfree(media_hlist);
ib_started = 0;
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:39:04
|
In current TIPC locking policy, tipc_net_lock protects two domains:
'node' and 'bearer' sub-domains. When one link instance is got from
"active_links" array in node sub-domain, tipc_net_lock(read) must be
held. Correspondingly, individual bearers may change status within a
tipc_net_lock(read), and it needs tipc_net_lock(write) to remove/add
any bearers. Obviously, this locking policy makes the two sub-domains
bound together closely.
To simplify the locking policy, we use one fine grained config_muex
lock to proect all resources associated with bearer/media like
media_list, tipc_bearers and media_hlist etc. But, even if the change
is made, tipc_net_lock is not removed from bearer domain until RCU
locks are involved to protect these global lists like media_list,
tipc_bearers and media_hlist in the future.
Additionally both initialised and uninitialised processes of TIPC
module have to be adjusted due to the locking policy change, so
tipc_core_start_net() and tipc_core_stop_net() become obsolete.
Signed-off-by: Ying Xue <yin...@wi...>
---
net/tipc/bearer.c | 4 ++++
net/tipc/config.c | 4 ++--
net/tipc/config.h | 4 ++--
net/tipc/core.c | 37 ++++++-------------------------------
net/tipc/core.h | 1 -
net/tipc/eth_media.c | 5 ++++-
net/tipc/ib_media.c | 5 ++++-
net/tipc/net.c | 3 +++
8 files changed, 25 insertions(+), 38 deletions(-)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 9f03a93..5dd587b 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -85,6 +85,7 @@ int tipc_register_media(struct tipc_media *m_ptr)
{
int res = -EINVAL;
+ mutex_lock(&config_mutex);
write_lock_bh(&tipc_net_lock);
if ((strlen(m_ptr->name) + 1) > TIPC_MAX_MEDIA_NAME)
@@ -104,6 +105,7 @@ int tipc_register_media(struct tipc_media *m_ptr)
res = 0;
exit:
write_unlock_bh(&tipc_net_lock);
+ mutex_unlock(&config_mutex);
if (res)
pr_warn("Media <%s> registration error\n", m_ptr->name);
return res;
@@ -114,6 +116,7 @@ void tipc_unregister_media(const struct tipc_media *m_ptr)
struct tipc_media *m;
u32 i;
+ mutex_lock(&config_mutex);
write_lock_bh(&tipc_net_lock);
for (i = 0; i < media_count; i++) {
m = media_list[i];
@@ -124,6 +127,7 @@ void tipc_unregister_media(const struct tipc_media *m_ptr)
}
}
write_unlock_bh(&tipc_net_lock);
+ mutex_unlock(&config_mutex);
}
/**
diff --git a/net/tipc/config.c b/net/tipc/config.c
index c301a9a..dc39fc4 100644
--- a/net/tipc/config.c
+++ b/net/tipc/config.c
@@ -42,7 +42,7 @@
#define REPLY_TRUNCATED "<truncated>\n"
-static DEFINE_MUTEX(config_mutex);
+DEFINE_MUTEX(config_mutex);
static struct tipc_server cfgsrv;
static const void *req_tlv_area; /* request message TLV area */
@@ -181,7 +181,7 @@ static struct sk_buff *cfg_set_own_addr(void)
if (tipc_own_addr)
return tipc_cfg_reply_error_string(TIPC_CFG_NOT_SUPPORTED
" (cannot change node address once assigned)");
- tipc_core_start_net(addr);
+ tipc_net_start(addr);
return tipc_cfg_reply_none();
}
diff --git a/net/tipc/config.h b/net/tipc/config.h
index 1f252f3..3c079c2 100644
--- a/net/tipc/config.h
+++ b/net/tipc/config.h
@@ -37,10 +37,10 @@
#ifndef _TIPC_CONFIG_H
#define _TIPC_CONFIG_H
-/* ---------------------------------------------------------------------- */
-
#include "link.h"
+extern struct mutex config_mutex;
+
struct sk_buff *tipc_cfg_reply_alloc(int payload_size);
int tipc_cfg_append_tlv(struct sk_buff *buf, int tlv_type,
void *tlv_data, int tlv_data_size);
diff --git a/net/tipc/core.c b/net/tipc/core.c
index fd4eeea..395167d 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -77,41 +77,13 @@ struct sk_buff *tipc_buf_acquire(u32 size)
}
/**
- * tipc_core_stop_net - shut down TIPC networking sub-systems
+ * tipc_core_stop - switch TIPC from SINGLE NODE to NOT RUNNING mode
*/
-static void tipc_core_stop_net(void)
+static void tipc_core_stop(void)
{
tipc_net_stop();
tipc_eth_media_stop();
tipc_ib_media_stop();
-}
-
-/**
- * start_net - start TIPC networking sub-systems
- */
-int tipc_core_start_net(unsigned long addr)
-{
- int res;
-
- tipc_net_start(addr);
- res = tipc_eth_media_start();
- if (res < 0)
- goto err;
- res = tipc_ib_media_start();
- if (res < 0)
- goto err;
- return res;
-
-err:
- tipc_core_stop_net();
- return res;
-}
-
-/**
- * tipc_core_stop - switch TIPC from SINGLE NODE to NOT RUNNING mode
- */
-static void tipc_core_stop(void)
-{
tipc_netlink_stop();
tipc_handler_stop();
tipc_cfg_stop();
@@ -146,6 +118,10 @@ static int tipc_core_start(void)
res = tipc_subscr_start();
if (!res)
res = tipc_cfg_init();
+ if (!res)
+ res = tipc_eth_media_start();
+ if (!res)
+ res = tipc_ib_media_start();
if (res)
tipc_core_stop();
@@ -178,7 +154,6 @@ static int __init tipc_init(void)
static void __exit tipc_exit(void)
{
- tipc_core_stop_net();
tipc_core_stop();
pr_info("Deactivated\n");
}
diff --git a/net/tipc/core.h b/net/tipc/core.h
index bef34ea..5402044 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -90,7 +90,6 @@ extern int tipc_random __read_mostly;
/*
* Routines available to privileged subsystems
*/
-int tipc_core_start_net(unsigned long);
int tipc_handler_start(void);
void tipc_handler_stop(void);
int tipc_netlink_start(void);
diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index 0480299..2d4458c 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -36,7 +36,7 @@
#include "core.h"
#include "bearer.h"
-#include "net.h"
+#include "config.h"
#define ETH_ADDR_OFFSET 4 /* message header offset of MAC address */
@@ -214,10 +214,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
if (!net_eq(dev_net(dev), &init_net))
return NOTIFY_DONE;
+ mutex_lock(&config_mutex);
read_lock_bh(&tipc_net_lock);
eb_ptr = media_get(dev->ifindex);
if (!eb_ptr) {
read_unlock_bh(&tipc_net_lock);
+ mutex_unlock(&config_mutex);
return NOTIFY_DONE; /* bearer had been disabled */
}
@@ -249,6 +251,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
break;
}
read_unlock_bh(&tipc_net_lock);
+ mutex_unlock(&config_mutex);
return NOTIFY_OK;
}
diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
index 916f60e..9decb3d 100644
--- a/net/tipc/ib_media.c
+++ b/net/tipc/ib_media.c
@@ -41,7 +41,7 @@
#include <linux/if_infiniband.h>
#include "core.h"
#include "bearer.h"
-#include "net.h"
+#include "config.h"
/**
* struct ib_media - Infiniband media data structure
@@ -218,10 +218,12 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
if (!net_eq(dev_net(dev), &init_net))
return NOTIFY_DONE;
+ mutex_lock(&config_mutex);
read_lock_bh(&tipc_net_lock);
ib_ptr = media_get(dev->ifindex);
if (!ib_ptr) {
read_unlock_bh(&tipc_net_lock);
+ mutex_unlock(&config_mutex);
return NOTIFY_DONE; /* bearer had been disabled */
}
@@ -253,6 +255,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
break;
}
read_unlock_bh(&tipc_net_lock);
+ mutex_unlock(&config_mutex);
return NOTIFY_OK;
}
diff --git a/net/tipc/net.c b/net/tipc/net.c
index 7d305ec..94f4e45 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -195,11 +195,14 @@ void tipc_net_stop(void)
if (!tipc_own_addr)
return;
+
+ mutex_lock(&config_mutex);
write_lock_bh(&tipc_net_lock);
tipc_bearer_stop();
tipc_bclink_stop();
list_for_each_entry_safe(node, t_node, &tipc_node_list, list)
tipc_node_delete(node);
write_unlock_bh(&tipc_net_lock);
+ mutex_unlock(&config_mutex);
pr_info("Left network mode\n");
}
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:39:06
|
In tipc_bearer struct there has usr_handle member pointing to media
object; in media struct, there is also a similar pointer called bearer
pointing to bearer object. When they access each other, one has to
use its pointer to access peer. But as the life cycle of both objects
is always same, now we let media struct contain bearer object directly,
having their relationship simpler.
Signed-off-by: Ying Xue <yin...@wi...>
---
net/tipc/bearer.c | 18 ++++++------------
net/tipc/bearer.h | 4 +---
net/tipc/eth_media.c | 42 ++++++++++++++++++++++--------------------
net/tipc/ib_media.c | 42 ++++++++++++++++++++++--------------------
4 files changed, 51 insertions(+), 55 deletions(-)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 5b32254..763a488 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -384,22 +384,17 @@ restart:
goto exit;
}
- b_ptr = kzalloc(sizeof(*b_ptr), GFP_ATOMIC);
- if (!b_ptr) {
- res = -ENOMEM;
+ b_ptr = m_ptr->enable_media(name);
+ if (IS_ERR(b_ptr)) {
+ res = PTR_ERR(b_ptr);
+ pr_warn("Bearer <%s> rejected, enable failure (%d)\n",
+ name, -res);
goto exit;
}
strcpy(b_ptr->name, name);
atomic_set(&b_ptr->blocked, 0);
smp_mb();
- res = m_ptr->enable_media(b_ptr);
- if (res) {
- pr_warn("Bearer <%s> rejected, enable failure (%d)\n",
- name, -res);
- goto exit;
- }
-
b_ptr->identity = bearer_id;
b_ptr->media = m_ptr;
b_ptr->tolerance = m_ptr->tolerance;
@@ -472,7 +467,6 @@ static void bearer_disable(struct tipc_bearer *b_ptr)
}
spin_lock_bh(&b_ptr->lock);
- b_ptr->media->disable_media(b_ptr);
list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
tipc_link_delete(l_ptr);
}
@@ -483,7 +477,7 @@ static void bearer_disable(struct tipc_bearer *b_ptr)
if (temp_req)
tipc_disc_delete(temp_req);
- kfree(b_ptr);
+ b_ptr->media->disable_media(b_ptr);
}
int tipc_disable_bearer(const char *name)
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 757eda9..6c6b3b5 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -91,7 +91,7 @@ struct tipc_media {
int (*send_msg)(struct sk_buff *buf,
struct tipc_bearer *b_ptr,
struct tipc_media_addr *dest);
- int (*enable_media)(struct tipc_bearer *b_ptr);
+ struct tipc_bearer *(*enable_media)(const char *name);
void (*disable_media)(struct tipc_bearer *b_ptr);
int (*addr2str)(struct tipc_media_addr *a, char *str_buf, int str_size);
int (*addr2msg)(struct tipc_media_addr *a, char *msg_area);
@@ -106,7 +106,6 @@ struct tipc_media {
/**
* struct tipc_bearer - TIPC bearer structure
- * @usr_handle: pointer to additional media-specific information about bearer
* @mtu: max packet size bearer can support
* @blocked: non-zero if bearer is blocked
* @lock: spinlock for controlling access to bearer
@@ -128,7 +127,6 @@ struct tipc_media {
* care of initializing all other fields.
*/
struct tipc_bearer {
- void *usr_handle; /* initalized by media */
u32 mtu; /* initalized by media */
atomic_t blocked;
struct tipc_media_addr addr; /* initalized by media */
diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index 2d4458c..8ad9e60 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -42,12 +42,12 @@
/**
* struct eth_media - Ethernet bearer data structure
- * @bearer: ptr to associated "generic" bearer structure
+ * @bearer: generic bearer structure
* @hlist: Ethernet media hash chain
* @dev: ptr to associated Ethernet network device
*/
struct eth_media {
- struct tipc_bearer *bearer;
+ struct tipc_bearer bearer;
struct hlist_node hlist;
struct net_device *dev;
};
@@ -101,7 +101,7 @@ static int send_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr,
if (!clone)
return 0;
- dev = ((struct eth_media *)(tb_ptr->usr_handle))->dev;
+ dev = ((struct eth_media *)(tb_ptr))->dev;
delta = dev->hard_header_len - skb_headroom(buf);
if ((delta > 0) &&
@@ -141,7 +141,7 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
if (likely(eb_ptr)) {
if (likely(buf->pkt_type <= PACKET_BROADCAST)) {
buf->next = NULL;
- tipc_recv_msg(buf, eb_ptr->bearer);
+ tipc_recv_msg(buf, (struct tipc_bearer *)eb_ptr);
read_unlock_bh(&tipc_net_lock);
return NET_RX_SUCCESS;
}
@@ -154,27 +154,27 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
/**
* enable_media - attach TIPC bearer to an Ethernet interface
*/
-static int enable_media(struct tipc_bearer *tb_ptr)
+static struct tipc_bearer *enable_media(const char *name)
{
struct net_device *dev;
struct eth_media *eb_ptr;
- char *driver_name = strchr((const char *)tb_ptr->name, ':') + 1;
+ struct tipc_bearer *tb_ptr;
+ char *driver_name = strchr(name, ':') + 1;
/* Find device with specified name */
dev = dev_get_by_name(&init_net, driver_name);
if (!dev)
- return -ENODEV;
+ return ERR_PTR(-ENODEV);
/* Create Ethernet bearer for device */
eb_ptr = kzalloc(sizeof(*eb_ptr), GFP_ATOMIC);
if (!eb_ptr)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
eb_ptr->dev = dev;
/* Associate TIPC bearer with Ethernet bearer */
- eb_ptr->bearer = tb_ptr;
- tb_ptr->usr_handle = (void *)eb_ptr;
+ tb_ptr = (struct tipc_bearer *)eb_ptr;
memset(tb_ptr->bcast_addr.value, 0, sizeof(tb_ptr->bcast_addr.value));
memcpy(tb_ptr->bcast_addr.value, dev->broadcast, ETH_ALEN);
tb_ptr->bcast_addr.media_id = TIPC_MEDIA_TYPE_ETH;
@@ -182,7 +182,7 @@ static int enable_media(struct tipc_bearer *tb_ptr)
tb_ptr->mtu = dev->mtu;
eth_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr);
hlist_add_head(&eb_ptr->hlist, media_hashfn(dev->ifindex));
- return 0;
+ return tb_ptr;
}
/**
@@ -192,7 +192,7 @@ static int enable_media(struct tipc_bearer *tb_ptr)
*/
static void disable_media(struct tipc_bearer *tb_ptr)
{
- struct eth_media *eb_ptr = (struct eth_media *)tb_ptr->usr_handle;
+ struct eth_media *eb_ptr = (struct eth_media *)tb_ptr;
hlist_del(&eb_ptr->hlist);
dev_put(eb_ptr->dev);
@@ -210,6 +210,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
{
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
struct eth_media *eb_ptr;
+ struct tipc_bearer *b_ptr;
if (!net_eq(dev_net(dev), &init_net))
return NOTIFY_DONE;
@@ -223,30 +224,31 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
return NOTIFY_DONE; /* bearer had been disabled */
}
- eb_ptr->bearer->mtu = dev->mtu;
+ b_ptr = (struct tipc_bearer *)eb_ptr;
+ b_ptr->mtu = dev->mtu;
switch (evt) {
case NETDEV_CHANGE:
if (netif_carrier_ok(dev))
- atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
+ atomic_add_unless(&b_ptr->blocked, -1, 0);
else
- tipc_block_bearer(eb_ptr->bearer);
+ tipc_block_bearer(b_ptr);
break;
case NETDEV_UP:
- atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
+ atomic_add_unless(&b_ptr->blocked, -1, 0);
break;
case NETDEV_DOWN:
- tipc_block_bearer(eb_ptr->bearer);
+ tipc_block_bearer(b_ptr);
break;
case NETDEV_CHANGEMTU:
case NETDEV_CHANGEADDR:
- tipc_block_bearer(eb_ptr->bearer);
- atomic_add_unless(&eb_ptr->bearer->blocked, -1, 0);
+ tipc_block_bearer(b_ptr);
+ atomic_add_unless(&b_ptr->blocked, -1, 0);
break;
case NETDEV_UNREGISTER:
case NETDEV_CHANGENAME:
read_unlock_bh(&tipc_net_lock);
- tipc_disable_bearer(eb_ptr->bearer->name);
+ tipc_disable_bearer(b_ptr->name);
read_lock_bh(&tipc_net_lock);
break;
}
diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
index 9decb3d..8981ded 100644
--- a/net/tipc/ib_media.c
+++ b/net/tipc/ib_media.c
@@ -45,13 +45,13 @@
/**
* struct ib_media - Infiniband media data structure
- * @bearer: ptr to associated "generic" bearer structure
+ * @bearer: generic bearer structure
* @hlist: Infiniband media hash chain
* @dev: ptr to associated Infiniband network device
*/
struct ib_media {
- struct tipc_bearer *bearer;
+ struct tipc_bearer bearer;
struct hlist_node hlist;
struct net_device *dev;
};
@@ -105,7 +105,7 @@ static int send_msg(struct sk_buff *buf, struct tipc_bearer *tb_ptr,
if (!clone)
return 0;
- dev = ((struct ib_media *)(tb_ptr->usr_handle))->dev;
+ dev = ((struct ib_media *)(tb_ptr))->dev;
delta = dev->hard_header_len - skb_headroom(buf);
if ((delta > 0) &&
@@ -145,7 +145,7 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
if (likely(ib_ptr)) {
if (likely(buf->pkt_type <= PACKET_BROADCAST)) {
buf->next = NULL;
- tipc_recv_msg(buf, ib_ptr->bearer);
+ tipc_recv_msg(buf, (struct tipc_bearer *)ib_ptr);
read_unlock_bh(&tipc_net_lock);
return NET_RX_SUCCESS;
}
@@ -158,27 +158,27 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
/**
* enable_media - attach TIPC bearer to an InfiniBand interface
*/
-static int enable_media(struct tipc_bearer *tb_ptr)
+static struct tipc_bearer *enable_media(const char *name)
{
struct net_device *dev;
struct ib_media *ib_ptr;
- char *driver_name = strchr((const char *)tb_ptr->name, ':') + 1;
+ struct tipc_bearer *tb_ptr;
+ char *driver_name = strchr(name, ':') + 1;
/* Find device with specified name */
dev = dev_get_by_name(&init_net, driver_name);
if (!dev)
- return -ENODEV;
+ return ERR_PTR(-ENODEV);
/* Create InfiniBand bearer for device */
ib_ptr = kzalloc(sizeof(*ib_ptr), GFP_ATOMIC);
if (!ib_ptr)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
ib_ptr->dev = dev;
/* Associate TIPC bearer with InfiniBand bearer */
- ib_ptr->bearer = tb_ptr;
- tb_ptr->usr_handle = (void *)ib_ptr;
+ tb_ptr = (struct tipc_bearer *)ib_ptr;
memset(tb_ptr->bcast_addr.value, 0, sizeof(tb_ptr->bcast_addr.value));
memcpy(tb_ptr->bcast_addr.value, dev->broadcast, INFINIBAND_ALEN);
tb_ptr->bcast_addr.media_id = TIPC_MEDIA_TYPE_IB;
@@ -186,7 +186,7 @@ static int enable_media(struct tipc_bearer *tb_ptr)
tb_ptr->mtu = dev->mtu;
ib_media_addr_set(tb_ptr, &tb_ptr->addr, (char *)dev->dev_addr);
hlist_add_head(&ib_ptr->hlist, media_hashfn(dev->ifindex));
- return 0;
+ return tb_ptr;
}
/**
@@ -196,7 +196,7 @@ static int enable_media(struct tipc_bearer *tb_ptr)
*/
static void disable_media(struct tipc_bearer *tb_ptr)
{
- struct ib_media *ib_ptr = (struct ib_media *)tb_ptr->usr_handle;
+ struct ib_media *ib_ptr = (struct ib_media *)tb_ptr;
hlist_del(&ib_ptr->hlist);
dev_put(ib_ptr->dev);
@@ -214,6 +214,7 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
{
struct net_device *dev = netdev_notifier_info_to_dev(ptr);
struct ib_media *ib_ptr;
+ struct tipc_bearer *b_ptr;
if (!net_eq(dev_net(dev), &init_net))
return NOTIFY_DONE;
@@ -227,30 +228,31 @@ static int recv_notification(struct notifier_block *nb, unsigned long evt,
return NOTIFY_DONE; /* bearer had been disabled */
}
- ib_ptr->bearer->mtu = dev->mtu;
+ b_ptr = (struct tipc_bearer *)ib_ptr;
+ b_ptr->mtu = dev->mtu;
switch (evt) {
case NETDEV_CHANGE:
if (netif_carrier_ok(dev))
- atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
+ atomic_add_unless(&b_ptr->blocked, -1, 0);
else
- tipc_block_bearer(ib_ptr->bearer);
+ tipc_block_bearer(b_ptr);
break;
case NETDEV_UP:
- atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
+ atomic_add_unless(&b_ptr->blocked, -1, 0);
break;
case NETDEV_DOWN:
- tipc_block_bearer(ib_ptr->bearer);
+ tipc_block_bearer(b_ptr);
break;
case NETDEV_CHANGEMTU:
case NETDEV_CHANGEADDR:
- tipc_block_bearer(ib_ptr->bearer);
- atomic_add_unless(&ib_ptr->bearer->blocked, -1, 0);
+ tipc_block_bearer(b_ptr);
+ atomic_add_unless(&b_ptr->blocked, -1, 0);
break;
case NETDEV_UNREGISTER:
case NETDEV_CHANGENAME:
read_unlock_bh(&tipc_net_lock);
- tipc_disable_bearer(ib_ptr->bearer->name);
+ tipc_disable_bearer(b_ptr->name);
read_lock_bh(&tipc_net_lock);
break;
}
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:39:10
|
There are a mix of function prototypes with and without extern in the TIPC source code, however, even though we don't use extern keyword with declaration of function, it is present there from compiler view. Therefore, standardize on not using extern for all function prototypes. Additionally the change also refers to the commit(ID: 5c15257f). Signed-off-by: Ying Xue <yin...@wi...> --- net/tipc/core.h | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/net/tipc/core.h b/net/tipc/core.h index be72f8c..bef34ea 100644 --- a/net/tipc/core.h +++ b/net/tipc/core.h @@ -90,21 +90,21 @@ extern int tipc_random __read_mostly; /* * Routines available to privileged subsystems */ -extern int tipc_core_start_net(unsigned long); -extern int tipc_handler_start(void); -extern void tipc_handler_stop(void); -extern int tipc_netlink_start(void); -extern void tipc_netlink_stop(void); -extern int tipc_socket_init(void); -extern void tipc_socket_stop(void); -extern int tipc_sock_create_local(int type, struct socket **res); -extern void tipc_sock_release_local(struct socket *sock); -extern int tipc_sock_accept_local(struct socket *sock, - struct socket **newsock, int flags); +int tipc_core_start_net(unsigned long); +int tipc_handler_start(void); +void tipc_handler_stop(void); +int tipc_netlink_start(void); +void tipc_netlink_stop(void); +int tipc_socket_init(void); +void tipc_socket_stop(void); +int tipc_sock_create_local(int type, struct socket **res); +void tipc_sock_release_local(struct socket *sock); +int tipc_sock_accept_local(struct socket *sock, struct socket **newsock, + int flags); #ifdef CONFIG_SYSCTL -extern int tipc_register_sysctl(void); -extern void tipc_unregister_sysctl(void); +int tipc_register_sysctl(void); +void tipc_unregister_sysctl(void); #else #define tipc_register_sysctl() 0 #define tipc_unregister_sysctl() @@ -201,6 +201,6 @@ static inline struct tipc_msg *buf_msg(struct sk_buff *skb) return (struct tipc_msg *)skb->data; } -extern struct sk_buff *tipc_buf_acquire(u32 size); +struct sk_buff *tipc_buf_acquire(u32 size); #endif -- 1.7.9.5 |
|
From: Jon M. <ma...@do...> - 2013-10-20 20:38:25
|
On 09/30/2013 10:38 AM, Ying Xue wrote: > There are a mix of function prototypes with and without extern in the > TIPC source code, however, even though we don't use extern keyword > with declaration of function, it is present there from compiler view. > Therefore, standardize on not using extern for all function prototypes. > Additionally the change also refers to the commit(ID: 5c15257f). Joe Perches has already done this for us, so we can skip this patch. ///jon > > Signed-off-by: Ying Xue <yin...@wi...> > --- > net/tipc/core.h | 28 ++++++++++++++-------------- > 1 file changed, 14 insertions(+), 14 deletions(-) > > diff --git a/net/tipc/core.h b/net/tipc/core.h > index be72f8c..bef34ea 100644 > --- a/net/tipc/core.h > +++ b/net/tipc/core.h > @@ -90,21 +90,21 @@ extern int tipc_random __read_mostly; > /* > * Routines available to privileged subsystems > */ > -extern int tipc_core_start_net(unsigned long); > -extern int tipc_handler_start(void); > -extern void tipc_handler_stop(void); > -extern int tipc_netlink_start(void); > -extern void tipc_netlink_stop(void); > -extern int tipc_socket_init(void); > -extern void tipc_socket_stop(void); > -extern int tipc_sock_create_local(int type, struct socket **res); > -extern void tipc_sock_release_local(struct socket *sock); > -extern int tipc_sock_accept_local(struct socket *sock, > - struct socket **newsock, int flags); > +int tipc_core_start_net(unsigned long); > +int tipc_handler_start(void); > +void tipc_handler_stop(void); > +int tipc_netlink_start(void); > +void tipc_netlink_stop(void); > +int tipc_socket_init(void); > +void tipc_socket_stop(void); > +int tipc_sock_create_local(int type, struct socket **res); > +void tipc_sock_release_local(struct socket *sock); > +int tipc_sock_accept_local(struct socket *sock, struct socket **newsock, > + int flags); > > #ifdef CONFIG_SYSCTL > -extern int tipc_register_sysctl(void); > -extern void tipc_unregister_sysctl(void); > +int tipc_register_sysctl(void); > +void tipc_unregister_sysctl(void); > #else > #define tipc_register_sysctl() 0 > #define tipc_unregister_sysctl() > @@ -201,6 +201,6 @@ static inline struct tipc_msg *buf_msg(struct sk_buff *skb) > return (struct tipc_msg *)skb->data; > } > > -extern struct sk_buff *tipc_buf_acquire(u32 size); > +struct sk_buff *tipc_buf_acquire(u32 size); > > #endif |
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:39:09
|
As tipc_bearers will be protected by RCU lock, tipc_bearers is
defined as static array where the data of tipc_bearer struct is
located in each array element. But since static array has not seen
with RCU, tipc_bearers should be redefined to a pointer array where
a pointer pointed to dynamically allocated instance of tipc_bearer
struct is located in each array element, allowing the pointer
array to be protected with RCU.
Signed-off-by: Ying Xue <yin...@wi...>
---
net/tipc/bcast.c | 6 +++---
net/tipc/bearer.c | 53 ++++++++++++++++++++++++++++++++++++++---------------
net/tipc/bearer.h | 2 +-
3 files changed, 42 insertions(+), 19 deletions(-)
diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c
index c78f30c..0378fda 100644
--- a/net/tipc/bcast.c
+++ b/net/tipc/bcast.c
@@ -660,6 +660,7 @@ void tipc_bcbearer_sort(void)
{
struct tipc_bcbearer_pair *bp_temp = bcbearer->bpairs_temp;
struct tipc_bcbearer_pair *bp_curr;
+ struct tipc_bearer *b;
int b_index;
int pri;
@@ -669,9 +670,8 @@ void tipc_bcbearer_sort(void)
memset(bp_temp, 0, sizeof(bcbearer->bpairs_temp));
for (b_index = 0; b_index < MAX_BEARERS; b_index++) {
- struct tipc_bearer *b = &tipc_bearers[b_index];
-
- if (!b->active || !b->nodes.count)
+ b = bearer_list[b_index];
+ if (!b || !b->active || !b->nodes.count)
continue;
if (!bp_temp[b->priority].primary)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 5dd587b..5b32254 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -42,10 +42,9 @@
#define MAX_ADDR_STR 60
static struct tipc_media *media_list[MAX_MEDIA];
+struct tipc_bearer *bearer_list[MAX_BEARERS];
static u32 media_count;
-struct tipc_bearer tipc_bearers[MAX_BEARERS];
-
static void bearer_disable(struct tipc_bearer *b_ptr);
/**
@@ -228,8 +227,9 @@ struct tipc_bearer *tipc_bearer_find(const char *name)
struct tipc_bearer *b_ptr;
u32 i;
- for (i = 0, b_ptr = tipc_bearers; i < MAX_BEARERS; i++, b_ptr++) {
- if (b_ptr->active && (!strcmp(b_ptr->name, name)))
+ for (i = 0; i < MAX_BEARERS; i++) {
+ b_ptr = bearer_list[i];
+ if (b_ptr && b_ptr->active && (!strcmp(b_ptr->name, name)))
return b_ptr;
}
return NULL;
@@ -244,8 +244,9 @@ struct tipc_bearer *tipc_bearer_find_interface(const char *if_name)
char *b_if_name;
u32 i;
- for (i = 0, b_ptr = tipc_bearers; i < MAX_BEARERS; i++, b_ptr++) {
- if (!b_ptr->active)
+ for (i = 0; i < MAX_BEARERS; i++) {
+ b_ptr = bearer_list[i];
+ if (!b_ptr || !b_ptr->active)
continue;
b_if_name = strchr(b_ptr->name, ':') + 1;
if (!strcmp(b_if_name, if_name))
@@ -270,7 +271,9 @@ struct sk_buff *tipc_bearer_get_names(void)
read_lock_bh(&tipc_net_lock);
for (i = 0; i < media_count; i++) {
for (j = 0; j < MAX_BEARERS; j++) {
- b_ptr = &tipc_bearers[j];
+ b_ptr = bearer_list[j];
+ if (!b_ptr)
+ continue;
if (b_ptr->active && (b_ptr->media == media_list[i])) {
tipc_cfg_append_tlv(buf, TIPC_TLV_BEARER_NAME,
b_ptr->name,
@@ -354,17 +357,17 @@ restart:
bearer_id = MAX_BEARERS;
with_this_prio = 1;
for (i = MAX_BEARERS; i-- != 0; ) {
- if (!tipc_bearers[i].active) {
+ b_ptr = bearer_list[i];
+ if (!b_ptr || !b_ptr->active) {
bearer_id = i;
continue;
}
- if (!strcmp(name, tipc_bearers[i].name)) {
+ if (!strcmp(name, b_ptr->name)) {
pr_warn("Bearer <%s> rejected, already enabled\n",
name);
goto exit;
}
- if ((tipc_bearers[i].priority == priority) &&
- (++with_this_prio > 2)) {
+ if ((b_ptr->priority == priority) && (++with_this_prio > 2)) {
if (priority-- == 0) {
pr_warn("Bearer <%s> rejected, duplicate priority\n",
name);
@@ -381,7 +384,12 @@ restart:
goto exit;
}
- b_ptr = &tipc_bearers[bearer_id];
+ b_ptr = kzalloc(sizeof(*b_ptr), GFP_ATOMIC);
+ if (!b_ptr) {
+ res = -ENOMEM;
+ goto exit;
+ }
+
strcpy(b_ptr->name, name);
atomic_set(&b_ptr->blocked, 0);
smp_mb();
@@ -409,6 +417,9 @@ restart:
name);
goto exit;
}
+
+ bearer_list[bearer_id] = b_ptr;
+
pr_info("Enabled bearer <%s>, discovery domain %s, priority %u\n",
name,
tipc_addr_string_fill(addr_string, disc_domain), priority);
@@ -449,9 +460,17 @@ static void bearer_disable(struct tipc_bearer *b_ptr)
struct tipc_link *l_ptr;
struct tipc_link *temp_l_ptr;
struct tipc_link_req *temp_req;
+ u32 i;
pr_info("Disabling bearer <%s>\n", b_ptr->name);
atomic_add_unless(&b_ptr->blocked, 1, 1);
+ for (i = 0; i < MAX_BEARERS; i++) {
+ if (b_ptr == bearer_list[i]) {
+ bearer_list[i] = NULL;
+ break;
+ }
+ }
+
spin_lock_bh(&b_ptr->lock);
b_ptr->media->disable_media(b_ptr);
list_for_each_entry_safe(l_ptr, temp_l_ptr, &b_ptr->links, link_list) {
@@ -464,7 +483,7 @@ static void bearer_disable(struct tipc_bearer *b_ptr)
if (temp_req)
tipc_disc_delete(temp_req);
- memset(b_ptr, 0, sizeof(struct tipc_bearer));
+ kfree(b_ptr);
}
int tipc_disable_bearer(const char *name)
@@ -487,11 +506,15 @@ int tipc_disable_bearer(const char *name)
void tipc_bearer_stop(void)
{
+ struct tipc_bearer *b_ptr;
u32 i;
for (i = 0; i < MAX_BEARERS; i++) {
- if (tipc_bearers[i].active)
- bearer_disable(&tipc_bearers[i]);
+ b_ptr = bearer_list[i];
+ if (b_ptr && b_ptr->active) {
+ bearer_disable(b_ptr);
+ bearer_list[i] = NULL;
+ }
}
media_count = 0;
}
diff --git a/net/tipc/bearer.h b/net/tipc/bearer.h
index 280c23d..757eda9 100644
--- a/net/tipc/bearer.h
+++ b/net/tipc/bearer.h
@@ -154,7 +154,7 @@ struct tipc_bearer_names {
struct tipc_link;
-extern struct tipc_bearer tipc_bearers[];
+extern struct tipc_bearer *bearer_list[];
/*
* TIPC routines available to supported media types
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:39:11
|
When the 'links' list of tipc_bearer structure is traversed in tipc_block_bearer() to reset all links owned by the bearer or it's iterated in bearer_disable() to delete all links attached to the bearer, the list is always protected by bearer lock. But when one new link entry is inserted into the "links" list when a link is created, the list isn't protected by bearer lock. Obviously, it's unsafe for us. Signed-off-by: Ying Xue <yin...@wi...> --- net/tipc/discover.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/tipc/discover.c b/net/tipc/discover.c index 8f86fcf..f4c3682 100644 --- a/net/tipc/discover.c +++ b/net/tipc/discover.c @@ -157,6 +157,8 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr) if (!n_ptr) return; } + + spin_lock_bh(&b_ptr->lock); tipc_node_lock(n_ptr); /* Prepare to validate requesting node's signature and media address */ @@ -245,6 +247,7 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr) } exit: tipc_node_unlock(n_ptr); + spin_unlock_bh(&b_ptr->lock); } /** -- 1.7.9.5 |
|
From: Ying X. <yin...@wi...> - 2013-09-30 08:39:14
|
A trivial optimization is made for tipc_disc_recv_msg() by adding a
common unwind section to reduce duplicated code.
Signed-off-by: Ying Xue <yin...@wi...>
---
net/tipc/discover.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/net/tipc/discover.c b/net/tipc/discover.c
index d8b49fb..8f86fcf 100644
--- a/net/tipc/discover.c
+++ b/net/tipc/discover.c
@@ -199,8 +199,7 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr)
/* delayed rediscovery */
} else {
disc_dupl_alert(b_ptr, orig, &media_addr);
- tipc_node_unlock(n_ptr);
- return;
+ goto exit;
}
n_ptr->signature = signature;
}
@@ -218,8 +217,7 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr)
if (addr_mismatch) {
if (tipc_link_is_up(link)) {
disc_dupl_alert(b_ptr, orig, &media_addr);
- tipc_node_unlock(n_ptr);
- return;
+ goto exit;
} else {
memcpy(&link->media_addr, &media_addr,
sizeof(media_addr));
@@ -230,10 +228,8 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr)
/* Create a link endpoint for this bearer, if necessary */
if (!link) {
link = tipc_link_create(n_ptr, b_ptr, &media_addr);
- if (!link) {
- tipc_node_unlock(n_ptr);
- return;
- }
+ if (!link)
+ goto exit;
}
/* Accept discovery message & send response, if necessary */
@@ -247,7 +243,7 @@ void tipc_disc_recv_msg(struct sk_buff *buf, struct tipc_bearer *b_ptr)
kfree_skb(rbuf);
}
}
-
+exit:
tipc_node_unlock(n_ptr);
}
--
1.7.9.5
|
|
From: Erik H. <eri...@er...> - 2013-10-07 08:50:15
|
On Mon, Sep 30, 2013 at 04:38:21PM +0800, Ying Xue wrote: > As tipc_bearers will be protected by RCU lock, tipc_bearers is > defined as static array where the data of tipc_bearer struct is > located in each array element. But since static array has not seen > with RCU, tipc_bearers should be redefined to a pointer array where > a pointer pointed to dynamically allocated instance of tipc_bearer > struct is located in each array element, allowing the pointer > array to be protected with RCU. > > Signed-off-by: Ying Xue <yin...@wi...> I find it hard to understand from the commitmsg what you're doing, and for what purpose. Maybe rephrase it to something like: "As part of the effort to introduce RCU protection for the bearer list, we first need to change it to a list of pointers." Maybe a little short though.. //E |
|
From: Ying X. <yin...@wi...> - 2013-10-09 02:57:12
|
On 10/07/2013 04:50 PM, Erik Hugne wrote: > On Mon, Sep 30, 2013 at 04:38:21PM +0800, Ying Xue wrote: >> As tipc_bearers will be protected by RCU lock, tipc_bearers is >> defined as static array where the data of tipc_bearer struct is >> located in each array element. But since static array has not seen >> with RCU, tipc_bearers should be redefined to a pointer array where >> a pointer pointed to dynamically allocated instance of tipc_bearer >> struct is located in each array element, allowing the pointer >> array to be protected with RCU. >> >> Signed-off-by: Ying Xue <yin...@wi...> > > I find it hard to understand from the commitmsg what you're doing, and for what > purpose. Maybe rephrase it to something like: > My puspose is to mainly describe why we need to convert tipc_bearer array to bearer list :). Essentially its resaon is very simple because RCU lock cannot protect static array. > "As part of the effort to introduce RCU protection for the bearer list, > we first need to change it to a list of pointers." > > Maybe a little short though.. > OK, I will adopt your suggestion if nobody object to.. Regards, Ying > //E > |
Thanks for helping keep SourceForge clean.
X