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-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;
>>>> }
>>>
>>>
>
>
>
|
