Re: [tipc-discussion] [net-next v3 2/3] tipc: modify struct tipc_plist to be more versatile

2016-12-19 Thread Ying Xue 
On 12/13/2016 10:03 PM, Jon Maloy wrote:
>> I still prefer to use "tipc_u32" prefix because the function names are a
>> > bit too generic without "tipc". Especially when we analyze stack trace,
>> > common function names cause a bit inconvenience for us.
> That is what I used first, but some code lines became awkwardly long, so I 
> had to split if-clauses and function calls over two lines, something I 
> generally try to avoid.
> Also, remember that this is an internal function that is only called from 
> other functions having the "tipc_" prefix, so you will never be in in doubt 
> where the problem is if you see a stack dump.
>

OK, it's fine with me.

Acked-by: Ying Xue 

> ///jon
>


--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
tipc-discussion mailing list
tipc-discussion@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tipc-discussion

Re: [tipc-discussion] [net-next v3 2/3] tipc: modify struct tipc_plist to be more versatile

2016-12-13 Thread Jon Maloy 


> -Original Message-
> From: Ying Xue [mailto:ying@windriver.com]
> Sent: Tuesday, 13 December, 2016 06:04
> To: Jon Maloy ; tipc-discussion@lists.sourceforge.net;
> Parthasarathy Bhuvaragan 
> Cc: ma...@donjonn.com; thompa@gmail.com
> Subject: Re: [net-next v3 2/3] tipc: modify struct tipc_plist to be more 
> versatile
> 
> On 12/13/2016 06:42 AM, Jon Maloy wrote:
> > During multicast reception we currently use a simple linked list with
> > push/pop semantics to store port numbers.
> >
> > We now see a need for a more generic list for storing values of type
> > u32. We therefore make some modifications to this list, while replacing
> > the prefix 'tipc_plist_' with 'u32_'.
> 
> It's a shame that we cannot use interfaces defined in lib/plist.c,
> otherwise, it's unnecessary for us to implement by ourselves.
> 
> I still prefer to use "tipc_u32" prefix because the function names are a
> bit too generic without "tipc". Especially when we analyze stack trace,
> common function names cause a bit inconvenience for us.

That is what I used first, but some code lines became awkwardly long, so I had 
to split if-clauses and function calls over two lines, something I generally 
try to avoid.
Also, remember that this is an internal function that is only called from other 
functions having the "tipc_" prefix, so you will never be in in doubt where the 
problem is if you see a stack dump.

///jon

> 
> Regards,
> Ying
> 
>   We also add a couple of new
> > functions which will come to use in the next commits.
> >
> > Signed-off-by: Jon Maloy 
> > ---
> >  net/tipc/name_table.c | 100 ---
> ---
> >  net/tipc/name_table.h |  21 ---
> >  net/tipc/socket.c |   8 ++--
> >  3 files changed, 83 insertions(+), 46 deletions(-)
> >
> > diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> > index e190460..5a86df1 100644
> > --- a/net/tipc/name_table.c
> > +++ b/net/tipc/name_table.c
> > @@ -608,7 +608,7 @@ u32 tipc_nametbl_translate(struct net *net, u32 type,
> u32 instance,
> >   * Returns non-zero if any off-node ports overlap
> >   */
> >  int tipc_nametbl_mc_translate(struct net *net, u32 type, u32 lower, u32
> upper,
> > - u32 limit, struct tipc_plist *dports)
> > + u32 limit, struct list_head *dports)
> >  {
> > struct name_seq *seq;
> > struct sub_seq *sseq;
> > @@ -633,7 +633,7 @@ int tipc_nametbl_mc_translate(struct net *net, u32
> type, u32 lower, u32 upper,
> > info = sseq->info;
> > list_for_each_entry(publ, >node_list, node_list) {
> > if (publ->scope <= limit)
> > -   tipc_plist_push(dports, publ->ref);
> > +   u32_push(dports, publ->ref);
> > }
> >
> > if (info->cluster_list_size != info->node_list_size)
> > @@ -1022,40 +1022,84 @@ int tipc_nl_name_table_dump(struct sk_buff *skb,
> struct netlink_callback *cb)
> > return skb->len;
> >  }
> >
> > -void tipc_plist_push(struct tipc_plist *pl, u32 port)
> > +struct u32_item {
> > +   struct list_head list;
> > +   u32 value;
> > +};
> > +
> > +bool u32_find(struct list_head *l, u32 value)
> >  {
> > -   struct tipc_plist *nl;
> > +   struct u32_item *item;
> >
> > -   if (likely(!pl->port)) {
> > -   pl->port = port;
> > -   return;
> > +   list_for_each_entry(item, l, list) {
> > +   if (item->value == value)
> > +   return true;
> > }
> > -   if (pl->port == port)
> > -   return;
> > -   list_for_each_entry(nl, >list, list) {
> > -   if (nl->port == port)
> > -   return;
> > +   return false;
> > +}
> > +
> > +bool u32_push(struct list_head *l, u32 value)
> > +{
> > +   struct u32_item *item;
> > +
> > +   list_for_each_entry(item, l, list) {
> > +   if (item->value == value)
> > +   return false;
> > +   }
> > +   item = kmalloc(sizeof(*item), GFP_ATOMIC);
> > +   if (unlikely(!item))
> > +   return false;
> > +
> > +   item->value = value;
> > +   list_add(>list, l);
> > +   return true;
> > +}
> > +
> > +u32 u32_pop(struct list_head *l)
> > +{
> > +   struct u32_item *item;
> > +   u32 value = 0;
> > +
> > +   if (list_empty(l))
> > +   return 0;
> > +   item = list_first_entry(l, typeof(*item), list);
> > +   value = item->value;
> > +   list_del(>list);
> > +   kfree(item);
> > +   return value;
> > +}
> > +
> > +bool u32_del(struct list_head *l, u32 value)
> > +{
> > +   struct u32_item *item, *tmp;
> > +
> > +   list_for_each_entry_safe(item, tmp, l, list) {
> > +   if (item->value != value)
> > +   continue;
> > +   list_del(>list);
> > +   kfree(item);
> > +   return true;
> > }
> > -   nl = kmalloc(sizeof(*nl), GFP_ATOMIC);
> > -   if (nl) {
> > -

Re: [tipc-discussion] [net-next v3 2/3] tipc: modify struct tipc_plist to be more versatile

2016-12-13 Thread Ying Xue 
On 12/13/2016 06:42 AM, Jon Maloy wrote:
> During multicast reception we currently use a simple linked list with
> push/pop semantics to store port numbers.
>
> We now see a need for a more generic list for storing values of type
> u32. We therefore make some modifications to this list, while replacing
> the prefix 'tipc_plist_' with 'u32_'.

It's a shame that we cannot use interfaces defined in lib/plist.c, 
otherwise, it's unnecessary for us to implement by ourselves.

I still prefer to use "tipc_u32" prefix because the function names are a 
bit too generic without "tipc". Especially when we analyze stack trace, 
common function names cause a bit inconvenience for us.

Regards,
Ying

  We also add a couple of new
> functions which will come to use in the next commits.
>
> Signed-off-by: Jon Maloy 
> ---
>  net/tipc/name_table.c | 100 
> --
>  net/tipc/name_table.h |  21 ---
>  net/tipc/socket.c |   8 ++--
>  3 files changed, 83 insertions(+), 46 deletions(-)
>
> diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
> index e190460..5a86df1 100644
> --- a/net/tipc/name_table.c
> +++ b/net/tipc/name_table.c
> @@ -608,7 +608,7 @@ u32 tipc_nametbl_translate(struct net *net, u32 type, u32 
> instance,
>   * Returns non-zero if any off-node ports overlap
>   */
>  int tipc_nametbl_mc_translate(struct net *net, u32 type, u32 lower, u32 
> upper,
> -   u32 limit, struct tipc_plist *dports)
> +   u32 limit, struct list_head *dports)
>  {
>   struct name_seq *seq;
>   struct sub_seq *sseq;
> @@ -633,7 +633,7 @@ int tipc_nametbl_mc_translate(struct net *net, u32 type, 
> u32 lower, u32 upper,
>   info = sseq->info;
>   list_for_each_entry(publ, >node_list, node_list) {
>   if (publ->scope <= limit)
> - tipc_plist_push(dports, publ->ref);
> + u32_push(dports, publ->ref);
>   }
>
>   if (info->cluster_list_size != info->node_list_size)
> @@ -1022,40 +1022,84 @@ int tipc_nl_name_table_dump(struct sk_buff *skb, 
> struct netlink_callback *cb)
>   return skb->len;
>  }
>
> -void tipc_plist_push(struct tipc_plist *pl, u32 port)
> +struct u32_item {
> + struct list_head list;
> + u32 value;
> +};
> +
> +bool u32_find(struct list_head *l, u32 value)
>  {
> - struct tipc_plist *nl;
> + struct u32_item *item;
>
> - if (likely(!pl->port)) {
> - pl->port = port;
> - return;
> + list_for_each_entry(item, l, list) {
> + if (item->value == value)
> + return true;
>   }
> - if (pl->port == port)
> - return;
> - list_for_each_entry(nl, >list, list) {
> - if (nl->port == port)
> - return;
> + return false;
> +}
> +
> +bool u32_push(struct list_head *l, u32 value)
> +{
> + struct u32_item *item;
> +
> + list_for_each_entry(item, l, list) {
> + if (item->value == value)
> + return false;
> + }
> + item = kmalloc(sizeof(*item), GFP_ATOMIC);
> + if (unlikely(!item))
> + return false;
> +
> + item->value = value;
> + list_add(>list, l);
> + return true;
> +}
> +
> +u32 u32_pop(struct list_head *l)
> +{
> + struct u32_item *item;
> + u32 value = 0;
> +
> + if (list_empty(l))
> + return 0;
> + item = list_first_entry(l, typeof(*item), list);
> + value = item->value;
> + list_del(>list);
> + kfree(item);
> + return value;
> +}
> +
> +bool u32_del(struct list_head *l, u32 value)
> +{
> + struct u32_item *item, *tmp;
> +
> + list_for_each_entry_safe(item, tmp, l, list) {
> + if (item->value != value)
> + continue;
> + list_del(>list);
> + kfree(item);
> + return true;
>   }
> - nl = kmalloc(sizeof(*nl), GFP_ATOMIC);
> - if (nl) {
> - nl->port = port;
> - list_add(>list, >list);
> + return false;
> +}
> +
> +void u32_list_purge(struct list_head *l)
> +{
> + struct u32_item *item, *tmp;
> +
> + list_for_each_entry_safe(item, tmp, l, list) {
> + list_del(>list);
> + kfree(item);
>   }
>  }
>
> -u32 tipc_plist_pop(struct tipc_plist *pl)
> +int u32_list_len(struct list_head *l)
>  {
> - struct tipc_plist *nl;
> - u32 port = 0;
> + struct u32_item *item;
> + int i = 0;
>
> - if (likely(list_empty(>list))) {
> - port = pl->port;
> - pl->port = 0;
> - return port;
> + list_for_each_entry(item, l, list) {
> + i++;
>   }
> - nl = list_first_entry(>list, typeof(*nl), list);
> - port = nl->port;
> - list_del(>list);
> - kfree(nl);
> - return port;
> + return i;
>  }
> diff --git