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