Menu
▾
▴
Re: [tipc-discussion] [PATCH net-next v3 10/12] tipc: make media structure contain tipc_bearer object
|
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;
>> }
>
>
>
|
