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