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