[netsniff-ng] Re: [PATCH] flowtop: Replace single linked list by list_head from list.h

2017-01-13 Thread Vadim Kochan
On Thu, Jan 12, 2017 at 5:20 PM, Tobias Klauser  wrote:
> On 2017-01-12 at 15:54:31 +0100, Vadim Kochan  wrote:
>> On Thu, Jan 12, 2017 at 4:28 PM, Tobias Klauser  wrote:
> [...]
>> >>  enum flow_direction {
>> >> @@ -355,15 +357,15 @@ static inline struct flow_entry 
>> >> *flow_entry_xalloc(void)
>> >>  static inline void flow_entry_xfree(struct flow_entry *n)
>> >>  {
>> >>   if (n->ct)
>> >> - nfct_destroy(n->ct);
>> >> + xfree(n->ct);
>> >
>> > This would leak memory allocated internally in struct nf_contrack, no?
>> > What's the reason for this change?
>>
>> nfct_destroy(ct) may fail if we free entry after nfct event processing
>> (our free is defered now because of RCU),
>> so using just free(x) is safer because we do just nfct_clone(ct) which
>> just allocated another nfct_conntrack entry for us,
>> but ofcourse it would be better to have something nfct_free(x), so I
>> agree this is not nice but safer.
>
> As long as it causes memory to be leaked IMO it is equally bad.
>
> If flow_entry_xfree() and thus nfct_destroy() is called via the
> call_rcu() wrapper I proposed, it should no longer be unsafe to call
> nfct_destroy()

I send separate v2 (I already send new version but w/o v2 prefix a changelog).

there should be no any leak as now nfct_conntrack will be not freed by
nfct API because I return NFCT_CB_STOLEN,
and it will be freed (destroyed) in defered rcu call.

I checked via valgrind that there is really memleak but after lookup
geoip record which is allocated by
libGeoIP but is not freed by flowtop. But it needs to be fixed in
separate patch.

-- 
You received this message because you are subscribed to the Google Groups 
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


[netsniff-ng] Re: [PATCH] flowtop: Replace single linked list by list_head from list.h

2017-01-12 Thread Tobias Klauser
On 2017-01-12 at 15:54:31 +0100, Vadim Kochan  wrote:
> On Thu, Jan 12, 2017 at 4:28 PM, Tobias Klauser  wrote:
[...]
> >>  enum flow_direction {
> >> @@ -355,15 +357,15 @@ static inline struct flow_entry 
> >> *flow_entry_xalloc(void)
> >>  static inline void flow_entry_xfree(struct flow_entry *n)
> >>  {
> >>   if (n->ct)
> >> - nfct_destroy(n->ct);
> >> + xfree(n->ct);
> >
> > This would leak memory allocated internally in struct nf_contrack, no?
> > What's the reason for this change?
> 
> nfct_destroy(ct) may fail if we free entry after nfct event processing
> (our free is defered now because of RCU),
> so using just free(x) is safer because we do just nfct_clone(ct) which
> just allocated another nfct_conntrack entry for us,
> but ofcourse it would be better to have something nfct_free(x), so I
> agree this is not nice but safer.

As long as it causes memory to be leaked IMO it is equally bad.

If flow_entry_xfree() and thus nfct_destroy() is called via the
call_rcu() wrapper I proposed, it should no longer be unsafe to call
nfct_destroy()

-- 
You received this message because you are subscribed to the Google Groups 
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


[netsniff-ng] Re: [PATCH] flowtop: Replace single linked list by list_head from list.h

2017-01-12 Thread Vadim Kochan
On Thu, Jan 12, 2017 at 4:28 PM, Tobias Klauser  wrote:
> Thanks for the patch and sorry for the delay in reply.
>
> I'm not too familiar with the RCU list interface, but from a quick
> glance this looks OK in general. A few remarks below.
>
> Daniel, if you have time could you maybe have a quick look?
>
> On 2017-01-09 at 07:26:07 +0100, Vadim Kochan  wrote:
>> list.h provides generic Linux-like linked list API which also supports
>> RCU list operations.
>>
>> Also additionally was removed the spinlock which is not needed for
>> RCU-list operations, for the list_del_rcu(...) case it is needed
>> additionally call synchronize_rcu(...) before free the flow entry.
>>
>> Because of full RCU support now flows are freed after grace-period via
>> calling synchronize_rcu() and these removed-and-ready-to-free entries
>> are kept in separate struct list_head.
>
> Do we really need the separate free list? Couldn't we add a struct
> rcu_head to struct flow_entry instead and then use something along the
> lines of:
>
> static void flow_entry_xfree_rcu(struct rcu_head *rcu)
> {
> struct flow_entry *entry = caa_container_of(rcu, struct flow_entry, 
> rcu_head);
> flow_entry_free(entry);
> }
>
> call_rcu(>rcu_head, free_entry_rcu);
>
> to free the entries after the RCU grace period?

Hm, OK I will look at this, for me it was simpler to use separate list head:)

>
>> Signed-off-by: Vadim Kochan 
>
>> ---
>>  flowtop.c | 121 
>> +++---
>>  1 file changed, 44 insertions(+), 77 deletions(-)
>>
>> diff --git a/flowtop.c b/flowtop.c
>> index f48f5c8..a5ac2bb 100644
>> --- a/flowtop.c
>>+++ b/flowtop.c
>
> The #include "locking.h" should be removed as non of the symbols defined
> there are used anymore in flowtop.c
>
>> @@ -51,6 +51,9 @@
>>  #endif
>>
>>  struct flow_entry {
>> + struct list_head entry;
>> + struct list_head free;
>> +
>>   uint32_t flow_id, use, status;
>>   uint8_t  l3_proto, l4_proto;
>>   uint32_t ip4_src_addr, ip4_dst_addr;
>> @@ -65,7 +68,6 @@ struct flow_entry {
>>   char city_src[128], city_dst[128];
>>   char rev_dns_src[256], rev_dns_dst[256];
>>   char procname[256];
>> - struct flow_entry *next;
>>   int inode;
>>   unsigned int procnum;
>>   bool is_visible;
>> @@ -78,8 +80,8 @@ struct flow_entry {
>>  };
>>
>>  struct flow_list {
>> - struct flow_entry *head;
>> - struct spinlock lock;
>> + struct list_head head;
>> + struct list_head free;
>>  };
>>
>>  enum flow_direction {
>> @@ -355,15 +357,15 @@ static inline struct flow_entry 
>> *flow_entry_xalloc(void)
>>  static inline void flow_entry_xfree(struct flow_entry *n)
>>  {
>>   if (n->ct)
>> - nfct_destroy(n->ct);
>> + xfree(n->ct);
>
> This would leak memory allocated internally in struct nf_contrack, no?
> What's the reason for this change?

nfct_destroy(ct) may fail if we free entry after nfct event processing
(our free is defered now because of RCU),
so using just free(x) is safer because we do just nfct_clone(ct) which
just allocated another nfct_conntrack entry for us,
but ofcourse it would be better to have something nfct_free(x), so I
agree this is not nice but safer.

>
>>   xfree(n);
>>  }
>>
>>  static inline void flow_list_init(struct flow_list *fl)
>>  {
>> - fl->head = NULL;
>> - spinlock_init(>lock);
>> + INIT_LIST_HEAD(>head);
>> + INIT_LIST_HEAD(>free);
>>  }
>>
>>  static inline bool nfct_is_dns(const struct nf_conntrack *ct)
>> @@ -392,84 +394,60 @@ static void flow_list_new_entry(struct flow_list *fl, 
>> const struct nf_conntrack
>>   flow_entry_from_ct(n, ct);
>>   flow_entry_get_extended(n);
>>
>> - rcu_assign_pointer(n->next, fl->head);
>> - rcu_assign_pointer(fl->head, n);
>> + list_add_rcu(>entry, >head);
>>
>>   n->is_visible = true;
>>  }
>>
>> -static struct flow_entry *flow_list_find_id(struct flow_list *fl,
>> - uint32_t id)
>> +static struct flow_entry *flow_list_find_id(struct flow_list *fl, uint32_t 
>> id)
>>  {
>> - struct flow_entry *n = rcu_dereference(fl->head);
>> + struct flow_entry *n;
>>
>> - while (n != NULL) {
>> + list_for_each_entry_rcu(n, >head, entry) {
> q
>>   if (n->flow_id == id)
>>   return n;
>> -
>> - n = rcu_dereference(n->next);
>>   }
>>
>>   return NULL;
>>  }
>>
>> -static struct flow_entry *flow_list_find_prev_id(const struct flow_list *fl,
>> -  uint32_t id)
>> +static void flow_list_del_entry(struct flow_list *fl, const struct 
>> nf_conntrack *ct)
>>  {
>> - struct flow_entry *prev = rcu_dereference(fl->head), *next;
>> -
>> - if (prev->flow_id == id)
>> - return NULL;
>> -
>> - while ((next = rcu_dereference(prev->next)) != NULL) {
>> -

[netsniff-ng] Re: [PATCH] flowtop: Replace single linked list by list_head from list.h

2017-01-12 Thread Tobias Klauser
Thanks for the patch and sorry for the delay in reply.

I'm not too familiar with the RCU list interface, but from a quick
glance this looks OK in general. A few remarks below.

Daniel, if you have time could you maybe have a quick look?

On 2017-01-09 at 07:26:07 +0100, Vadim Kochan  wrote:
> list.h provides generic Linux-like linked list API which also supports
> RCU list operations.
> 
> Also additionally was removed the spinlock which is not needed for
> RCU-list operations, for the list_del_rcu(...) case it is needed
> additionally call synchronize_rcu(...) before free the flow entry.
> 
> Because of full RCU support now flows are freed after grace-period via
> calling synchronize_rcu() and these removed-and-ready-to-free entries
> are kept in separate struct list_head.

Do we really need the separate free list? Couldn't we add a struct
rcu_head to struct flow_entry instead and then use something along the
lines of:

static void flow_entry_xfree_rcu(struct rcu_head *rcu)
{
struct flow_entry *entry = caa_container_of(rcu, struct flow_entry, 
rcu_head);
flow_entry_free(entry);
}

call_rcu(>rcu_head, free_entry_rcu);

to free the entries after the RCU grace period?

> Signed-off-by: Vadim Kochan 

> ---
>  flowtop.c | 121 
> +++---
>  1 file changed, 44 insertions(+), 77 deletions(-)
> 
> diff --git a/flowtop.c b/flowtop.c
> index f48f5c8..a5ac2bb 100644
> --- a/flowtop.c
>+++ b/flowtop.c

The #include "locking.h" should be removed as non of the symbols defined
there are used anymore in flowtop.c

> @@ -51,6 +51,9 @@
>  #endif
>  
>  struct flow_entry {
> + struct list_head entry;
> + struct list_head free;
> +
>   uint32_t flow_id, use, status;
>   uint8_t  l3_proto, l4_proto;
>   uint32_t ip4_src_addr, ip4_dst_addr;
> @@ -65,7 +68,6 @@ struct flow_entry {
>   char city_src[128], city_dst[128];
>   char rev_dns_src[256], rev_dns_dst[256];
>   char procname[256];
> - struct flow_entry *next;
>   int inode;
>   unsigned int procnum;
>   bool is_visible;
> @@ -78,8 +80,8 @@ struct flow_entry {
>  };
>  
>  struct flow_list {
> - struct flow_entry *head;
> - struct spinlock lock;
> + struct list_head head;
> + struct list_head free;
>  };
>  
>  enum flow_direction {
> @@ -355,15 +357,15 @@ static inline struct flow_entry *flow_entry_xalloc(void)
>  static inline void flow_entry_xfree(struct flow_entry *n)
>  {
>   if (n->ct)
> - nfct_destroy(n->ct);
> + xfree(n->ct);

This would leak memory allocated internally in struct nf_contrack, no?
What's the reason for this change?

>   xfree(n);
>  }
>  
>  static inline void flow_list_init(struct flow_list *fl)
>  {
> - fl->head = NULL;
> - spinlock_init(>lock);
> + INIT_LIST_HEAD(>head);
> + INIT_LIST_HEAD(>free);
>  }
>  
>  static inline bool nfct_is_dns(const struct nf_conntrack *ct)
> @@ -392,84 +394,60 @@ static void flow_list_new_entry(struct flow_list *fl, 
> const struct nf_conntrack
>   flow_entry_from_ct(n, ct);
>   flow_entry_get_extended(n);
>  
> - rcu_assign_pointer(n->next, fl->head);
> - rcu_assign_pointer(fl->head, n);
> + list_add_rcu(>entry, >head);
>  
>   n->is_visible = true;
>  }
>  
> -static struct flow_entry *flow_list_find_id(struct flow_list *fl,
> - uint32_t id)
> +static struct flow_entry *flow_list_find_id(struct flow_list *fl, uint32_t 
> id)
>  {
> - struct flow_entry *n = rcu_dereference(fl->head);
> + struct flow_entry *n;
>  
> - while (n != NULL) {
> + list_for_each_entry_rcu(n, >head, entry) {
q
>   if (n->flow_id == id)
>   return n;
> -
> - n = rcu_dereference(n->next);
>   }
>  
>   return NULL;
>  }
>  
> -static struct flow_entry *flow_list_find_prev_id(const struct flow_list *fl,
> -  uint32_t id)
> +static void flow_list_del_entry(struct flow_list *fl, const struct 
> nf_conntrack *ct)
>  {
> - struct flow_entry *prev = rcu_dereference(fl->head), *next;
> -
> - if (prev->flow_id == id)
> - return NULL;
> -
> - while ((next = rcu_dereference(prev->next)) != NULL) {
> - if (next->flow_id == id)
> - return prev;
> + struct flow_entry *n;
>  
> - prev = next;
> + n = flow_list_find_id(fl, nfct_get_attr_u32(ct, ATTR_ID));
> + if (n) {
> + list_del_rcu(>entry);
> + list_add_rcu(>free, >free);

Here would be the call_rcu() outlined above (instead of list_add_rcu)

>   }
> -
> - return NULL;
>  }
>  
> -static void flow_list_destroy_entry(struct flow_list *fl,
> - const struct nf_conntrack *ct)
> +static void flow_list_clear(struct flow_list *fl)
>  {
> - struct flow_entry *n1, *n2;
> -