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

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

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

[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

[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 0/4] Introduce new pcap io API for pcap r/w accesses

[Tobias Klauser]

On 2017-01-12 at 13:59:05 +0100, Vadim Kochan  wrote:
> On Wed, Dec 14, 2016 at 11:33 PM, Vadim Kochan  wrote:
> > On Wed, Dec 14, 2016 at 11:26 PM, Tobias Klauser  
> > wrote:
> >> On 2016-12-12 at 22:09:52 +0100, Vadim Kochan  wrote:
> >>> Add new pcap io API to make pcap read/write accesses more
> >>> simpler and generic. Added pcap_io & pcap_packet struct's to
> >>> keep some internal pcap state like magic, link type & packet header
> >>> instead of to pass them like parameters and keep it all within 
> >>> netsniff-ng.c.
> >>>
> >>> Also such approach might be used to unify sniffing from ring buffer via
> >>> pcap io API similary as it is done with regular files.
> >>>
> >>> Some fast-path sensitive or setter/getter functions were inlined in 
> >>> pcap_io.h.
> >>
> >> This series touches very sensitive fast-path code. Before even reviewing
> >> it, I'd like to see performance measurments proofing that the your
> >> patches don't introduce any performance regressions and that inlining of
> >> these functions indeed does the job.
> >>
> >> Thanks!
> >
> > Hm, yeah, I need to think how to perform this on my laptop ...
> 
> I will try to measure time execution of critical code via clock()
> syscall for both versions if it will be OK for you, if yes -
> would be it OK to add some #IFDEF's (ahhh not too nice probably) to
> have such calculation persistent in the code ?

I think it should also be possible to test basic performance by just
invoking netsniff-ng under perf and then comparing profiles.

Of course you're also free to instrument the code for performance
measurments locally, but I'd rather not put any instrumentation code
into the upstream repo.

Thanks
Tobias

-- 
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 0/4] Introduce new pcap io API for pcap r/w accesses

[Vadim Kochan]

On Wed, Dec 14, 2016 at 11:33 PM, Vadim Kochan  wrote:
> On Wed, Dec 14, 2016 at 11:26 PM, Tobias Klauser  wrote:
>> On 2016-12-12 at 22:09:52 +0100, Vadim Kochan  wrote:
>>> Add new pcap io API to make pcap read/write accesses more
>>> simpler and generic. Added pcap_io & pcap_packet struct's to
>>> keep some internal pcap state like magic, link type & packet header
>>> instead of to pass them like parameters and keep it all within 
>>> netsniff-ng.c.
>>>
>>> Also such approach might be used to unify sniffing from ring buffer via
>>> pcap io API similary as it is done with regular files.
>>>
>>> Some fast-path sensitive or setter/getter functions were inlined in 
>>> pcap_io.h.
>>
>> This series touches very sensitive fast-path code. Before even reviewing
>> it, I'd like to see performance measurments proofing that the your
>> patches don't introduce any performance regressions and that inlining of
>> these functions indeed does the job.
>>
>> Thanks!
>
> Hm, yeah, I need to think how to perform this on my laptop ...

I will try to measure time execution of critical code via clock()
syscall for both versions if it will be OK for you, if yes -
would be it OK to add some #IFDEF's (ahhh not too nice probably) to
have such calculation persistent in the code ?

Regards,
Vadim Kochan

-- 
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.