Menu
▾
▴
tipc-discussion
|
From: Erik H. <eri...@er...> - 2013-10-07 09:01:10
|
On Mon, Sep 30, 2013 at 04:38:22PM +0800, Ying Xue wrote: > 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...> I suggest the following: The lifecycle of media and bearers are the same, and their relationship can be simplified by directly aggregating the tipc_bearer structure in the tipc_media. This eliminates the need for media/bearer access pointers in respective structs. //E |
|
From: Ying X. <yin...@wi...> - 2013-10-09 02:58:26
|
On 10/07/2013 05:01 PM, Erik Hugne wrote: > On Mon, Sep 30, 2013 at 04:38:22PM +0800, Ying Xue wrote: >> 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...> > > I suggest the following: > > The lifecycle of media and bearers are the same, and their relationship > can be simplified by directly aggregating the tipc_bearer structure in the > tipc_media. This eliminates the need for media/bearer access pointers in > respective structs. > Thanks, your description is pretty good :) Regards, Ying > //E > |
|
From: Ying X. <yin...@wi...> - 2013-10-16 09:11:30
|
Hi Jon, If you have time, please comment the version. Regards, Ying On 09/30/2013 04:38 PM, Ying Xue wrote: > 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(-) > |
|
From: Jon M. <ma...@do...> - 2013-10-20 20:32:42
|
On 09/30/2013 10:38 AM, Ying Xue wrote:
> 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.
Good observation. The reason for this was originally to have a strong separation between
the media specific code/structs and the generic ones. It should even be possible to
attach a driver external medium. We still need some separation, but I have no problem
with this change.
I am wondering if we should take the full step, and even aggregate tipc_media into
eth/ib_media. Of course this would mean some duplication of function pointers and
other fields, but it may actually give a positive cache effect. Since we will never have
many bearers (typically 2), saving memory is not a reason to not do this.
///jon
>
> 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;
> }
|
|
From: Ying X. <yin...@wi...> - 2013-10-21 09:47:20
|
On 10/21/2013 04:18 AM, Jon Maloy wrote:
> On 09/30/2013 10:38 AM, Ying Xue wrote:
>> 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.
>
> Good observation. The reason for this was originally to have a strong separation between
> the media specific code/structs and the generic ones. It should even be possible to
> attach a driver external medium. We still need some separation, but I have no problem
> with this change.
> I am wondering if we should take the full step, and even aggregate tipc_media into
> eth/ib_media. Of course this would mean some duplication of function pointers and
> other fields, but it may actually give a positive cache effect. Since we will never have
> many bearers (typically 2), saving memory is not a reason to not do this.
>
I am not sure whether I correctly understand your meanings. In my
opinion, this change already makes tipc_media being aggregated into
eth/ib_media because we now put tipc_bearer containing tipc_media
pointer into eth/ib_media. But your meaning sounds like:
struct eth_media {
struct tipc_bearer bearer;
+ struct tipc_media media;
struct hlist_node hlist;
struct net_device *dev;
};
struct tipc_bearer {
...
char name[TIPC_MAX_BEARER_NAME];
spinlock_t lock;
- struct tipc_media *media;
struct tipc_media_addr bcast_addr;
...
}
If so, code might become worse than before because the generic layers
like link and discovery modules don't know how to access methods
provided by one specific media with tipc_bearer pointer. Of course,
probaly I misunderstood your idea.
I agree with your another idea of removing media_list because it is only
useful when we query media names with TIPC_CMD_GET_BEARER_NAMES command.
But if media_list is removed, we have the following choices when coping
with TIPC_CMD_GET_BEARER_NAMES command:
1. Obsolete the TIPC_CMD_GET_BEARER_NAMES because the commend is not too
much meaningful for users in reality.
2. Diectly return query results of media name with tipc_bearers list
although the results might contain duplicated media names as the mapping
relationship between media and bearer is 1:N.
3. We still give the results of media names with tipc_bearers list, but
we filter the results to remove duplicated items before the command is
returned. But this change is a bit complex.
Please give comments again.
Thanks!
Regards,
Ying
> ///jon
>
>>
>> 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;
>> }
>
>
>
|
|
From: Jon M. <ma...@do...> - 2013-10-21 11:49:56
|
On 10/21/2013 11:46 AM, Ying Xue wrote:
> On 10/21/2013 04:18 AM, Jon Maloy wrote:
>> On 09/30/2013 10:38 AM, Ying Xue wrote:
>>> 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.
>> Good observation. The reason for this was originally to have a strong separation between
>> the media specific code/structs and the generic ones. It should even be possible to
>> attach a driver external medium. We still need some separation, but I have no problem
>> with this change.
>> I am wondering if we should take the full step, and even aggregate tipc_media into
>> eth/ib_media. Of course this would mean some duplication of function pointers and
>> other fields, but it may actually give a positive cache effect. Since we will never have
>> many bearers (typically 2), saving memory is not a reason to not do this.
>>
> I am not sure whether I correctly understand your meanings. In my
> opinion, this change already makes tipc_media being aggregated into
> eth/ib_media because we now put tipc_bearer containing tipc_media
> pointer into eth/ib_media. But your meaning sounds like:
>
> struct eth_media {
> struct tipc_bearer bearer;
> + struct tipc_media media;
> struct hlist_node hlist;
> struct net_device *dev;
> };
>
> struct tipc_bearer {
> ...
> char name[TIPC_MAX_BEARER_NAME];
> spinlock_t lock;
> - struct tipc_media *media;
> struct tipc_media_addr bcast_addr;
> ...
> }
Actually, this was what I meant, but i get your point.
We should rather do like this:
struct tipc_bearer {
...
char name[TIPC_MAX_BEARER_NAME];
spinlock_t lock;
- struct tipc_media *media;
+ struct tipc_media media;
struct tipc_media_addr bcast_addr;
...
}
The contents of tipc_media has to be filled in by eth_bearer/ib_bearer respectively,
but will become available from the generic code.
>
> If so, code might become worse than before because the generic layers
> like link and discovery modules don't know how to access methods
> provided by one specific media with tipc_bearer pointer. Of course,
> probaly I misunderstood your idea.
>
> I agree with your another idea of removing media_list because it is only
> useful when we query media names with TIPC_CMD_GET_BEARER_NAMES command.
>
> But if media_list is removed, we have the following choices when coping
> with TIPC_CMD_GET_BEARER_NAMES command:
>
> 1. Obsolete the TIPC_CMD_GET_BEARER_NAMES because the commend is not too
> much meaningful for users in reality.
>
> 2. Diectly return query results of media name with tipc_bearers list
> although the results might contain duplicated media names as the mapping
> relationship between media and bearer is 1:N.
>
> 3. We still give the results of media names with tipc_bearers list, but
> we filter the results to remove duplicated items before the command is
> returned. But this change is a bit complex.
I would prefer method 3, and I don't think it has to be that complex.
Something like:
for (i = 0; i < bearer_count; i++) {
if (strstr(buf,bearers[i].media.name))
continue;
tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
media_list[i]->name,
strlen(media_list[i]->name) + 1);
}
I.e. we filter out duplicates already from the beginning. I think this should work.
Regards
///jon
>
> Please give comments again.
>
> Thanks!
>
> Regards,
> Ying
>
>
>> ///jon
>>
>>> 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;
>>> }
>>
>>
|
|
From: Ying X. <yin...@wi...> - 2013-10-22 02:58:23
|
On 10/21/2013 07:49 PM, Jon Maloy wrote:
> On 10/21/2013 11:46 AM, Ying Xue wrote:
>> On 10/21/2013 04:18 AM, Jon Maloy wrote:
>>> On 09/30/2013 10:38 AM, Ying Xue wrote:
>>>> 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.
>>> Good observation. The reason for this was originally to have a strong separation between
>>> the media specific code/structs and the generic ones. It should even be possible to
>>> attach a driver external medium. We still need some separation, but I have no problem
>>> with this change.
>>> I am wondering if we should take the full step, and even aggregate tipc_media into
>>> eth/ib_media. Of course this would mean some duplication of function pointers and
>>> other fields, but it may actually give a positive cache effect. Since we will never have
>>> many bearers (typically 2), saving memory is not a reason to not do this.
>>>
>> I am not sure whether I correctly understand your meanings. In my
>> opinion, this change already makes tipc_media being aggregated into
>> eth/ib_media because we now put tipc_bearer containing tipc_media
>> pointer into eth/ib_media. But your meaning sounds like:
>>
>> struct eth_media {
>> struct tipc_bearer bearer;
>> + struct tipc_media media;
>> struct hlist_node hlist;
>> struct net_device *dev;
>> };
>>
>> struct tipc_bearer {
>> ...
>> char name[TIPC_MAX_BEARER_NAME];
>> spinlock_t lock;
>> - struct tipc_media *media;
>> struct tipc_media_addr bcast_addr;
>> ...
>> }
>
> Actually, this was what I meant, but i get your point.
> We should rather do like this:
>
> struct tipc_bearer {
> ...
> char name[TIPC_MAX_BEARER_NAME];
> spinlock_t lock;
> - struct tipc_media *media;
> + struct tipc_media media;
> struct tipc_media_addr bcast_addr;
> ...
> }
>
> The contents of tipc_media has to be filled in by eth_bearer/ib_bearer respectively,
> but will become available from the generic code.
>
Thanks for your clear explanation! But, sorry, I don't see where the
advantage of this change is. After the media_list is removed,
tipc_register_media() becomes meaningless. If media pointer is defined
in tipc_bearer, it can be set to one specific tipc_media object declared
in eth/ib_media statically when bearer/meida is enabled. Even if more
same bearers are enabled, the media poiners in tipc_bearer structures
point to the same tipc_media object, which can keep all relevant code
pretty clean. In contrast, if tipc_media instance is defined in
tipc_bearer, all methods as well as intial values in tipc_media must be
manually set each time bearer is enabled. Moreover, this change also
make tipc_bearer size bigger.
Regards,
Ying
>>
>> If so, code might become worse than before because the generic layers
>> like link and discovery modules don't know how to access methods
>> provided by one specific media with tipc_bearer pointer. Of course,
>> probaly I misunderstood your idea.
>>
>> I agree with your another idea of removing media_list because it is only
>> useful when we query media names with TIPC_CMD_GET_BEARER_NAMES command.
>>
>> But if media_list is removed, we have the following choices when coping
>> with TIPC_CMD_GET_BEARER_NAMES command:
>>
>> 1. Obsolete the TIPC_CMD_GET_BEARER_NAMES because the commend is not too
>> much meaningful for users in reality.
>>
>> 2. Diectly return query results of media name with tipc_bearers list
>> although the results might contain duplicated media names as the mapping
>> relationship between media and bearer is 1:N.
>>
>> 3. We still give the results of media names with tipc_bearers list, but
>> we filter the results to remove duplicated items before the command is
>> returned. But this change is a bit complex.
>
> I would prefer method 3, and I don't think it has to be that complex.
> Something like:
>
> for (i = 0; i < bearer_count; i++) {
> if (strstr(buf,bearers[i].media.name))
> continue;
> tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
> media_list[i]->name,
> strlen(media_list[i]->name) + 1);
> }
>
> I.e. we filter out duplicates already from the beginning. I think this should work.
>
> Regards
> ///jon
>
>>
>> Please give comments again.
>>
>> Thanks!
>>
>> Regards,
>> Ying
>>
>>
>>> ///jon
>>>
>>>> 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;
>>>> }
>>>
>>>
>
>
>
|
|
From: Jon M. <jon...@er...> - 2013-10-22 06:20:42
|
On 10/22/2013 04:57 AM, Ying Xue wrote:
> On 10/21/2013 07:49 PM, Jon Maloy wrote:
>> On 10/21/2013 11:46 AM, Ying Xue wrote:
>>> On 10/21/2013 04:18 AM, Jon Maloy wrote:
>>>> On 09/30/2013 10:38 AM, Ying Xue wrote:
>>>>> 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
[...]
>>
>> struct tipc_bearer {
>> ...
>> char name[TIPC_MAX_BEARER_NAME];
>> spinlock_t lock;
>> - struct tipc_media *media;
>> + struct tipc_media media;
>> struct tipc_media_addr bcast_addr;
>> ...
>> }
>>
>> The contents of tipc_media has to be filled in by eth_bearer/ib_bearer respectively,
>> but will become available from the generic code.
>>
>
> Thanks for your clear explanation! But, sorry, I don't see where the
> advantage of this change is. After the media_list is removed,
> tipc_register_media() becomes meaningless. If media pointer is defined
> in tipc_bearer, it can be set to one specific tipc_media object declared
> in eth/ib_media statically when bearer/meida is enabled. Even if more
> same bearers are enabled, the media poiners in tipc_bearer structures
> point to the same tipc_media object, which can keep all relevant code
> pretty clean.
This is also a good way to do it. At least we get rid of the media_list array,
which simplifies our structures.
In contrast, if tipc_media instance is defined in
> tipc_bearer, all methods as well as intial values in tipc_media must be
> manually set each time bearer is enabled. Moreover, this change also
> make tipc_bearer size bigger.
We have very few bearer instances (typically two for now), so I don't
think memory consumption is an issue. And, as I stated yesterday, it may have
a positive effect on cache behaviour.
But I have no strong feelings about either solution. Do as you find best.
My only concern about both solutios is that the bearer code doesn't any
longer know the number of registered media, as you pointed out yesterday.
I would prefer that we don't disable the -m switch in tipc-config, since I
think it can be useful sometimes.
My suggestion from yesterday about using strstr() is not perfect either,
since you may find a media name as a substring within another.
(Imagine the media names "gretha" and "eth" to use a somewhat artificial
example). Maybe we can come up with something better hers?
///jon
>
>
> Regards,
> Ying
>
>>>
>>> If so, code might become worse than before because the generic layers
>>> like link and discovery modules don't know how to access methods
>>> provided by one specific media with tipc_bearer pointer. Of course,
>>> probaly I misunderstood your idea.
>>>
>>> I agree with your another idea of removing media_list because it is only
>>> useful when we query media names with TIPC_CMD_GET_BEARER_NAMES command.
>>>
>>> But if media_list is removed, we have the following choices when coping
>>> with TIPC_CMD_GET_BEARER_NAMES command:
>>>
>>> 1. Obsolete the TIPC_CMD_GET_BEARER_NAMES because the commend is not too
>>> much meaningful for users in reality.
>>>
>>> 2. Diectly return query results of media name with tipc_bearers list
>>> although the results might contain duplicated media names as the mapping
>>> relationship between media and bearer is 1:N.
>>>
>>> 3. We still give the results of media names with tipc_bearers list, but
>>> we filter the results to remove duplicated items before the command is
>>> returned. But this change is a bit complex.
>>
>> I would prefer method 3, and I don't think it has to be that complex.
>> Something like:
>>
>> for (i = 0; i < bearer_count; i++) {
>> if (strstr(buf,bearers[i].media.name))
>> continue;
>> tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
>> media_list[i]->name,
>> strlen(media_list[i]->name) + 1);
>> }
>>
>> I.e. we filter out duplicates already from the beginning. I think this should work.
>>
>> Regards
>> ///jon
>>
>>>
>>> Please give comments again.
>>>
>>> Thanks!
>>>
>>> Regards,
>>> Ying
>>>
>>>
>>>> ///jon
>>>>
>>>>> 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;
>>>>> }
>>>>
>>>>
>>
>>
>>
>
|
|
From: Jon M. <jon...@er...> - 2013-10-22 06:21:15
|
On 10/22/2013 04:57 AM, Ying Xue wrote:
> On 10/21/2013 07:49 PM, Jon Maloy wrote:
>> On 10/21/2013 11:46 AM, Ying Xue wrote:
>>> On 10/21/2013 04:18 AM, Jon Maloy wrote:
>>>> On 09/30/2013 10:38 AM, Ying Xue wrote:
>>>>> 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
[...]
>>
>> struct tipc_bearer {
>> ...
>> char name[TIPC_MAX_BEARER_NAME];
>> spinlock_t lock;
>> - struct tipc_media *media;
>> + struct tipc_media media;
>> struct tipc_media_addr bcast_addr;
>> ...
>> }
>>
>> The contents of tipc_media has to be filled in by eth_bearer/ib_bearer respectively,
>> but will become available from the generic code.
>>
>
> Thanks for your clear explanation! But, sorry, I don't see where the
> advantage of this change is. After the media_list is removed,
> tipc_register_media() becomes meaningless. If media pointer is defined
> in tipc_bearer, it can be set to one specific tipc_media object declared
> in eth/ib_media statically when bearer/meida is enabled. Even if more
> same bearers are enabled, the media poiners in tipc_bearer structures
> point to the same tipc_media object, which can keep all relevant code
> pretty clean.
This is also a good way to do it. At least we get rid of the media_list array,
which simplifies our structures.
In contrast, if tipc_media instance is defined in
> tipc_bearer, all methods as well as intial values in tipc_media must be
> manually set each time bearer is enabled. Moreover, this change also
> make tipc_bearer size bigger.
We have very few bearer instances (typically two for now), so I don't
think memory consumption is an issue. And, as I stated yesterday, it may have
a positive effect on cache behaviour.
But I have no strong feelings about either solution. Do as you find best.
My only concern about both solution is that the bearer code doesn't any
longer know the number of registered media, as you pointed out yesterday.
I would prefer that we don't disable the -m switch in tipc-config, since I
think it can be useful sometimes.
My suggestion from yesterday about using strstr() is not perfect either,
since you may find a media name as a substring within another.
(Imagine the media names "gretha" and "eth" to use a somewhat artificial
example). Maybe we can come up with something better hers?
///jon
>
>
> Regards,
> Ying
>
>>>
>>> If so, code might become worse than before because the generic layers
>>> like link and discovery modules don't know how to access methods
>>> provided by one specific media with tipc_bearer pointer. Of course,
>>> probaly I misunderstood your idea.
>>>
>>> I agree with your another idea of removing media_list because it is only
>>> useful when we query media names with TIPC_CMD_GET_BEARER_NAMES command.
>>>
>>> But if media_list is removed, we have the following choices when coping
>>> with TIPC_CMD_GET_BEARER_NAMES command:
>>>
>>> 1. Obsolete the TIPC_CMD_GET_BEARER_NAMES because the commend is not too
>>> much meaningful for users in reality.
>>>
>>> 2. Diectly return query results of media name with tipc_bearers list
>>> although the results might contain duplicated media names as the mapping
>>> relationship between media and bearer is 1:N.
>>>
>>> 3. We still give the results of media names with tipc_bearers list, but
>>> we filter the results to remove duplicated items before the command is
>>> returned. But this change is a bit complex.
>>
>> I would prefer method 3, and I don't think it has to be that complex.
>> Something like:
>>
>> for (i = 0; i < bearer_count; i++) {
>> if (strstr(buf,bearers[i].media.name))
>> continue;
>> tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
>> media_list[i]->name,
>> strlen(media_list[i]->name) + 1);
>> }
>>
>> I.e. we filter out duplicates already from the beginning. I think this should work.
>>
>> Regards
>> ///jon
>>
>>>
>>> Please give comments again.
>>>
>>> Thanks!
>>>
>>> Regards,
>>> Ying
>>>
>>>
>>>> ///jon
>>>>
>>>>> 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;
>>>>> }
>>>>
>>>>
>>
>>
>>
>
|
|
From: Ying X. <yin...@wi...> - 2013-10-22 09:52:11
|
On 10/22/2013 02:20 PM, Jon Maloy wrote:
> On 10/22/2013 04:57 AM, Ying Xue wrote:
>> On 10/21/2013 07:49 PM, Jon Maloy wrote:
>>> On 10/21/2013 11:46 AM, Ying Xue wrote:
>>>> On 10/21/2013 04:18 AM, Jon Maloy wrote:
>>>>> On 09/30/2013 10:38 AM, Ying Xue wrote:
>>>>>> 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
>
> [...]
>
>>>
>>> struct tipc_bearer {
>>> ...
>>> char name[TIPC_MAX_BEARER_NAME];
>>> spinlock_t lock;
>>> - struct tipc_media *media;
>>> + struct tipc_media media;
>>> struct tipc_media_addr bcast_addr;
>>> ...
>>> }
>>>
>>> The contents of tipc_media has to be filled in by eth_bearer/ib_bearer respectively,
>>> but will become available from the generic code.
>>>
>>
>> Thanks for your clear explanation! But, sorry, I don't see where the
>> advantage of this change is. After the media_list is removed,
>> tipc_register_media() becomes meaningless. If media pointer is defined
>> in tipc_bearer, it can be set to one specific tipc_media object declared
>> in eth/ib_media statically when bearer/meida is enabled. Even if more
>> same bearers are enabled, the media poiners in tipc_bearer structures
>> point to the same tipc_media object, which can keep all relevant code
>> pretty clean.
>
> This is also a good way to do it. At least we get rid of the media_list array,
> which simplifies our structures.
>
OK, I will remain tipc_media pointer in tipc_bearer.
>
>
> In contrast, if tipc_media instance is defined in
>> tipc_bearer, all methods as well as intial values in tipc_media must be
>> manually set each time bearer is enabled. Moreover, this change also
>> make tipc_bearer size bigger.
>
> We have very few bearer instances (typically two for now), so I don't
> think memory consumption is an issue. And, as I stated yesterday, it may have
> a positive effect on cache behaviour.
> But I have no strong feelings about either solution. Do as you find best.
>
> My only concern about both solution is that the bearer code doesn't any
> longer know the number of registered media, as you pointed out yesterday.
> I would prefer that we don't disable the -m switch in tipc-config, since I
> think it can be useful sometimes.
> My suggestion from yesterday about using strstr() is not perfect either,
> since you may find a media name as a substring within another.
> (Imagine the media names "gretha" and "eth" to use a somewhat artificial
> example). Maybe we can come up with something better hers?
>
Yes, your original proposal has a bit problem. In fact I cannot figure
out a simpler and smarter soluction too, but please see below piece of
code, which should meet our requirement although it's not simple.
char status[bearer_count];
char *name;
int found;
memset(status, 0, sizeof(status));
for (i = 0; i < bearer_count; i++) {
found = 0;
name = &bearers[i]->media.name;
for (j = 0; j < bearer_count; j++)) {
if (!status(j) &&
!strcmp(name, bearers[j]->media.name)) {
status(j) = 1;
found = 1;
}
}
if (found)
tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
name, strlen(name) + 1);
}
Regards,
Ying
> ///jon
>
>>
>>
>> Regards,
>> Ying
>>
>>>>
>>>> If so, code might become worse than before because the generic layers
>>>> like link and discovery modules don't know how to access methods
>>>> provided by one specific media with tipc_bearer pointer. Of course,
>>>> probaly I misunderstood your idea.
>>>>
>>>> I agree with your another idea of removing media_list because it is only
>>>> useful when we query media names with TIPC_CMD_GET_BEARER_NAMES command.
>>>>
>>>> But if media_list is removed, we have the following choices when coping
>>>> with TIPC_CMD_GET_BEARER_NAMES command:
>>>>
>>>> 1. Obsolete the TIPC_CMD_GET_BEARER_NAMES because the commend is not too
>>>> much meaningful for users in reality.
>>>>
>>>> 2. Diectly return query results of media name with tipc_bearers list
>>>> although the results might contain duplicated media names as the mapping
>>>> relationship between media and bearer is 1:N.
>>>>
>>>> 3. We still give the results of media names with tipc_bearers list, but
>>>> we filter the results to remove duplicated items before the command is
>>>> returned. But this change is a bit complex.
>>>
>>> I would prefer method 3, and I don't think it has to be that complex.
>>> Something like:
>>>
>>> for (i = 0; i < bearer_count; i++) {
>>> if (strstr(buf,bearers[i].media.name))
>>> continue;
>>> tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
>>> media_list[i]->name,
>>> strlen(media_list[i]->name) + 1);
>>> }
>>>
>>> I.e. we filter out duplicates already from the beginning. I think this should work.
>>>
>>> Regards
>>> ///jon
>>>
>>>>
>>>> Please give comments again.
>>>>
>>>> Thanks!
>>>>
>>>> Regards,
>>>> Ying
>>>>
>>>>
>>>>> ///jon
>>>>>
>>>>>> 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;
>>>>>> }
>>>>>
>>>>>
>>>
>>>
>>>
>>
>
>
>
|
|
From: Erik H. <eri...@er...> - 2013-10-24 13:07:23
|
On Tue, Oct 22, 2013 at 05:51:47PM +0800, Ying Xue wrote:
> >>>> I agree with your another idea of removing media_list because it is only
> >>>> useful when we query media names with TIPC_CMD_GET_BEARER_NAMES command.
I don't think the media list can be dropped so easily. We need to be able to
associate a media name with the appropriate callback when a bearer is enabled.
tipc_enable_bearer()
tipc_media_find()
/*iterate over media list until a media matching the input name is found*/
Or?
//E
|
|
From: Jon M. <jon...@er...> - 2013-10-24 13:27:17
|
> -----Original Message-----
> From: Ying Xue [mailto:yin...@wi...]
> Sent: October-22-13 11:52 AM
> To: Jon Maloy
> Cc: Jon Maloy; Pau...@wi...; Erik Hugne; tipc-
> dis...@li...
> Subject: Re: [PATCH net-next v3 10/12] tipc: make media structure contain
> tipc_bearer object
>
> On 10/22/2013 02:20 PM, Jon Maloy wrote:
> > On 10/22/2013 04:57 AM, Ying Xue wrote:
> >> On 10/21/2013 07:49 PM, Jon Maloy wrote:
> >>> On 10/21/2013 11:46 AM, Ying Xue wrote:
> >>>> On 10/21/2013 04:18 AM, Jon Maloy wrote:
> >>>>> On 09/30/2013 10:38 AM, Ying Xue wrote:
[...]
> >
>
> Yes, your original proposal has a bit problem. In fact I cannot figure out a
> simpler and smarter soluction too, but please see below piece of code, which
> should meet our requirement although it's not simple.
>
> char status[bearer_count];
> char *name;
> int found;
>
> memset(status, 0, sizeof(status));
>
> for (i = 0; i < bearer_count; i++) {
> found = 0;
> name = &bearers[i]->media.name;
> for (j = 0; j < bearer_count; j++)) {
> if (!status(j) &&
> !strcmp(name, bearers[j]->media.name)) {
> status(j) = 1;
> found = 1;
> }
> }
> if (found)
> tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
> name, strlen(name) + 1);
> }
>
> Regards,
> Ying
We both forgot about something basic here:
tipc-config -m must be able to respond with the registered media even when no
bearers are enabled, i.e. when bearer_count is zero. The purpose of this command
is to know which kind of interfaces we *can* enable. E.g., a user can no see that
Infiniband is not registered by default at module start.
So, we have two options:
1: Keep the current model, with a static array of pointers to the statically
allocated tipc_media (one per media type).
This array is filled in once and for all at module start, just like the struct
itself, and then never altered. It, just like the struct themselves, can be
shared between different kernel contexts, once we introduce this.
There should be no need need to lock protect neither the array nor the
structs, and there is no need for a media_deregister() routine, which
I think was the change that trigged this discussion.
The only purpose of the pointer array would be to read the media names
from tipc-config.
2: We register only the names of the media in a static array, and use
this as information source for tipc-config. No other coordination
between the media structs is needed.
As you will understand, I really want to keep the number of locks
to a minimum, even if they are RCU locks. A write-once, read-many,
never-delete, struct and array fulfils all criteria for not needing lock
protection.
What do you prefer? Other suggestions?
///jon
>
>
> > ///jon
> >
> >>
> >>
> >> Regards,
> >> Ying
> >>
> >>>>
> >>>> If so, code might become worse than before because the generic
> >>>> layers like link and discovery modules don't know how to access
> >>>> methods provided by one specific media with tipc_bearer pointer. Of
> >>>> course, probaly I misunderstood your idea.
> >>>>
> >>>> I agree with your another idea of removing media_list because it is
> >>>> only useful when we query media names with
> TIPC_CMD_GET_BEARER_NAMES command.
> >>>>
> >>>> But if media_list is removed, we have the following choices when
> >>>> coping with TIPC_CMD_GET_BEARER_NAMES command:
> >>>>
> >>>> 1. Obsolete the TIPC_CMD_GET_BEARER_NAMES because the
> commend is
> >>>> not too much meaningful for users in reality.
> >>>>
> >>>> 2. Diectly return query results of media name with tipc_bearers
> >>>> list although the results might contain duplicated media names as
> >>>> the mapping relationship between media and bearer is 1:N.
> >>>>
> >>>> 3. We still give the results of media names with tipc_bearers list,
> >>>> but we filter the results to remove duplicated items before the
> >>>> command is returned. But this change is a bit complex.
> >>>
> >>> I would prefer method 3, and I don't think it has to be that complex.
> >>> Something like:
> >>>
> >>> for (i = 0; i < bearer_count; i++) {
> >>> if (strstr(buf,bearers[i].media.name))
> >>> continue;
> >>> tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
> >>> media_list[i]->name,
> >>> strlen(media_list[i]->name) + 1);
> >>> }
> >>>
> >>> I.e. we filter out duplicates already from the beginning. I think this
> should work.
> >>>
> >>> Regards
> >>> ///jon
> >>>
> >>>>
> >>>> Please give comments again.
> >>>>
> >>>> Thanks!
> >>>>
> >>>> Regards,
> >>>> Ying
> >>>>
> >>>>
> >>>>> ///jon
> >>>>>
> >>>>>> 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;
> >>>>>> }
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >>
> >
> >
> >
|
|
From: Ying X. <yin...@wi...> - 2013-10-25 07:10:06
|
On 10/24/2013 09:27 PM, Jon Maloy wrote:
>
>
>> -----Original Message-----
>> From: Ying Xue [mailto:yin...@wi...]
>> Sent: October-22-13 11:52 AM
>> To: Jon Maloy
>> Cc: Jon Maloy; Pau...@wi...; Erik Hugne; tipc-
>> dis...@li...
>> Subject: Re: [PATCH net-next v3 10/12] tipc: make media structure contain
>> tipc_bearer object
>>
>> On 10/22/2013 02:20 PM, Jon Maloy wrote:
>>> On 10/22/2013 04:57 AM, Ying Xue wrote:
>>>> On 10/21/2013 07:49 PM, Jon Maloy wrote:
>>>>> On 10/21/2013 11:46 AM, Ying Xue wrote:
>>>>>> On 10/21/2013 04:18 AM, Jon Maloy wrote:
>>>>>>> On 09/30/2013 10:38 AM, Ying Xue wrote:
>
> [...]
>
>>>
>>
>> Yes, your original proposal has a bit problem. In fact I cannot figure out a
>> simpler and smarter soluction too, but please see below piece of code, which
>> should meet our requirement although it's not simple.
>>
>> char status[bearer_count];
>> char *name;
>> int found;
>>
>> memset(status, 0, sizeof(status));
>>
>> for (i = 0; i < bearer_count; i++) {
>> found = 0;
>> name = &bearers[i]->media.name;
>> for (j = 0; j < bearer_count; j++)) {
>> if (!status(j) &&
>> !strcmp(name, bearers[j]->media.name)) {
>> status(j) = 1;
>> found = 1;
>> }
>> }
>> if (found)
>> tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
>> name, strlen(name) + 1);
>> }
>>
>> Regards,
>> Ying
>
> We both forgot about something basic here:
>
> tipc-config -m must be able to respond with the registered media even when no
> bearers are enabled, i.e. when bearer_count is zero. The purpose of this command
> is to know which kind of interfaces we *can* enable. E.g., a user can no see that
> Infiniband is not registered by default at module start.
>
Agreed.
> So, we have two options:
> 1: Keep the current model, with a static array of pointers to the statically
> allocated tipc_media (one per media type).
> This array is filled in once and for all at module start, just like the struct
> itself, and then never altered. It, just like the struct themselves, can be
> shared between different kernel contexts, once we introduce this.
> There should be no need need to lock protect neither the array nor the
> structs, and there is no need for a media_deregister() routine, which
> I think was the change that trigged this discussion.
> The only purpose of the pointer array would be to read the media names
> from tipc-config.
>
OK. this also means that holding tipc_net_lock is unncessary in
tipc_register_media(), right? In other words, media_list[] is no needed
to be protected with tipc_net_lock. This also means it's unnecessary to
protect media_list[] with RCU. I agree with this.
Additionally, as the members like tolerance, priority and window in
tipc_media struct are protected by mutex_config after we remove
tipc_net_lock, they are safe too.
Regards,
Ying
> 2: We register only the names of the media in a static array, and use
> this as information source for tipc-config. No other coordination
> between the media structs is needed.
>
> As you will understand, I really want to keep the number of locks
> to a minimum, even if they are RCU locks. A write-once, read-many,
> never-delete, struct and array fulfils all criteria for not needing lock
> protection.
>
> What do you prefer? Other suggestions?
>
> ///jon
>
>
>>
>>
>>> ///jon
>>>
>>>>
>>>>
>>>> Regards,
>>>> Ying
>>>>
>>>>>>
>>>>>> If so, code might become worse than before because the generic
>>>>>> layers like link and discovery modules don't know how to access
>>>>>> methods provided by one specific media with tipc_bearer pointer. Of
>>>>>> course, probaly I misunderstood your idea.
>>>>>>
>>>>>> I agree with your another idea of removing media_list because it is
>>>>>> only useful when we query media names with
>> TIPC_CMD_GET_BEARER_NAMES command.
>>>>>>
>>>>>> But if media_list is removed, we have the following choices when
>>>>>> coping with TIPC_CMD_GET_BEARER_NAMES command:
>>>>>>
>>>>>> 1. Obsolete the TIPC_CMD_GET_BEARER_NAMES because the
>> commend is
>>>>>> not too much meaningful for users in reality.
>>>>>>
>>>>>> 2. Diectly return query results of media name with tipc_bearers
>>>>>> list although the results might contain duplicated media names as
>>>>>> the mapping relationship between media and bearer is 1:N.
>>>>>>
>>>>>> 3. We still give the results of media names with tipc_bearers list,
>>>>>> but we filter the results to remove duplicated items before the
>>>>>> command is returned. But this change is a bit complex.
>>>>>
>>>>> I would prefer method 3, and I don't think it has to be that complex.
>>>>> Something like:
>>>>>
>>>>> for (i = 0; i < bearer_count; i++) {
>>>>> if (strstr(buf,bearers[i].media.name))
>>>>> continue;
>>>>> tipc_cfg_append_tlv(buf, TIPC_TLV_MEDIA_NAME,
>>>>> media_list[i]->name,
>>>>> strlen(media_list[i]->name) + 1);
>>>>> }
>>>>>
>>>>> I.e. we filter out duplicates already from the beginning. I think this
>> should work.
>>>>>
>>>>> Regards
>>>>> ///jon
>>>>>
>>>>>>
>>>>>> Please give comments again.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> Regards,
>>>>>> Ying
>>>>>>
>>>>>>
>>>>>>> ///jon
>>>>>>>
>>>>>>>> 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;
>>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>
>
>
|
|
From: Ying X. <yin...@wi...> - 2013-10-25 10:40:53
|
v4 changes:
- remove patch #2 (tipc: involve tipc_unregister_media routine)
- remove patch #7 (tipc: remove extern keywords from function
declarations)
- rebase other patches on the latest net-next tree
- revise patch headers of patch #9 and patch #10 with Erik's
suggestions.
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
*** BLURB HERE ***
Ying Xue (10):
tipc: convert 'blocked' flag in struct tipc_bearer to atomic_t
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 list
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 | 92 +++++++++++------------
net/tipc/bearer.h | 11 +--
net/tipc/config.c | 4 +-
net/tipc/config.h | 4 +-
net/tipc/core.c | 37 ++-------
net/tipc/core.h | 1 -
net/tipc/discover.c | 22 +++---
net/tipc/eth_media.c | 204 +++++++++++++++++++++++++-------------------------
net/tipc/ib_media.c | 190 +++++++++++++++++++++++-----------------------
net/tipc/link.c | 14 ++--
net/tipc/net.c | 3 +
12 files changed, 278 insertions(+), 314 deletions(-)
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-10-25 10:40:54
|
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 e8153f6..0e7d50a 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: Ying X. <yin...@wi...> - 2013-10-25 10:40:57
|
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 17f9824..3bbd90b 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -404,7 +404,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);
@@ -416,7 +415,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 32d01e8..ffba0c5 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 5241a5b..8ed7358 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 0e7d50a..12c72d2 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-10-25 10:41:00
|
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 | 115 ++++++++++++++++++++++++++++---------------------- net/tipc/ib_media.c | 102 ++++++++++++++++++++++++++------------------ 2 files changed, 124 insertions(+), 93 deletions(-) diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c index ffba0c5..80850aa 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,32 @@ 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); + return res; + } + + eth_started = 1; return res; } @@ -382,5 +394,6 @@ void tipc_eth_media_stop(void) flush_scheduled_work(); unregister_netdevice_notifier(¬ifier); + kfree(media_hlist); eth_started = 0; } diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c index 8ed7358..7d35f47 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,32 @@ 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); + return res; + } + + ib_started = 1; return res; } @@ -378,5 +395,6 @@ void tipc_ib_media_stop(void) flush_scheduled_work(); unregister_netdevice_notifier(¬ifier); + kfree(media_hlist); ib_started = 0; } -- 1.7.9.5 |
|
From: Ying X. <yin...@wi...> - 2013-10-25 10:41:03
|
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 80850aa..7b867ae 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);
}
/**
@@ -380,6 +348,8 @@ int tipc_eth_media_start(void)
return res;
}
+ dev_add_pack(&tipc_packet_type);
+
eth_started = 1;
return res;
}
@@ -394,6 +364,7 @@ void tipc_eth_media_stop(void)
flush_scheduled_work();
unregister_netdevice_notifier(¬ifier);
+ dev_remove_pack(&tipc_packet_type);
kfree(media_hlist);
eth_started = 0;
}
diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c
index 7d35f47..1bb9bd5 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);
}
/**
@@ -381,6 +350,8 @@ int tipc_ib_media_start(void)
return res;
}
+ dev_add_pack(&tipc_packet_type);
+
ib_started = 1;
return res;
}
@@ -395,6 +366,7 @@ void tipc_ib_media_stop(void)
flush_scheduled_work();
unregister_netdevice_notifier(¬ifier);
+ dev_remove_pack(&tipc_packet_type);
kfree(media_hlist);
ib_started = 0;
}
--
1.7.9.5
|
|
From: Ying X. <yin...@wi...> - 2013-10-25 10:41:04
|
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 7b867ae..47aebb2 100644 --- a/net/tipc/eth_media.c +++ b/net/tipc/eth_media.c @@ -362,7 +362,6 @@ void tipc_eth_media_stop(void) if (!eth_started) return; - flush_scheduled_work(); unregister_netdevice_notifier(¬ifier); dev_remove_pack(&tipc_packet_type); kfree(media_hlist); diff --git a/net/tipc/ib_media.c b/net/tipc/ib_media.c index 1bb9bd5..78b1312 100644 --- a/net/tipc/ib_media.c +++ b/net/tipc/ib_media.c @@ -364,7 +364,6 @@ void tipc_ib_media_stop(void) if (!ib_started) return; - flush_scheduled_work(); unregister_netdevice_notifier(¬ifier); dev_remove_pack(&tipc_packet_type); kfree(media_hlist); -- 1.7.9.5 |
|
From: Ying X. <yin...@wi...> - 2013-10-25 10:41:08
|
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 | 2 ++
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, 23 insertions(+), 38 deletions(-)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 3bbd90b..52e6b92 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;
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 94895d4..9e45ac4 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 47aebb2..0569cce 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 78b1312..7f825cd 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-10-25 10:41:10
|
As part of the effort to introduce RCU protection for the bearer
list, we first need to change it to a list of pointers.
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 52e6b92..d5f9eee 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);
/**
@@ -209,8 +208,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;
@@ -225,8 +225,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))
@@ -251,7 +252,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,
@@ -335,17 +338,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);
@@ -362,7 +365,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();
@@ -390,6 +398,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);
@@ -430,9 +441,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) {
@@ -445,7 +464,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)
@@ -468,11 +487,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 8695d4a..6687ad2 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-10-25 10:41:14
|
The lifecycle of media and bearers are the same, and their relationship
can be simplified by directly aggregating the tipc_bearer structure in the
tipc_media. This eliminates the need for media/bearer access pointers in
respective structs.
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 d5f9eee..a9b0745 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -365,22 +365,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;
@@ -453,7 +448,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);
}
@@ -464,7 +458,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 6687ad2..9568c3a 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 0569cce..1cc7e3c 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 7f825cd..3bd00bb 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-10-25 10:41: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: Ying X. <yin...@wi...> - 2013-10-25 10:41:18
|
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: Jon M. <ma...@do...> - 2013-10-27 21:57:17
|
On 10/25/2013 06:40 AM, Ying Xue wrote:
> 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.
Ying,
I may have misunderstood something here, but to me it looks like this
patch is obsoleted by patch #8 in this series, where you
aggregate in the bearer struct in eth_media. I wouldn't mind
submitting this anyway, since tipc_net_lock is going away eventually,
but I suspect David M and others may make the same observation
and ask the same question.
///jon
>
> 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 17f9824..3bbd90b 100644
> --- a/net/tipc/bearer.c
> +++ b/net/tipc/bearer.c
> @@ -404,7 +404,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);
> @@ -416,7 +415,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 32d01e8..ffba0c5 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 5241a5b..8ed7358 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 0e7d50a..12c72d2 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);
> }
>
> /**
|
Thanks for helping keep SourceForge clean.
X