Re: [netsniff-ng] Re: [PATCH 00/15] trafgen: Support dinc & drnd for proto fields

[Vadim Kochan]

On Tue, Aug 9, 2016 at 5:11 PM, Tobias Klauser  wrote:
> Hi Vadim
>
> On 2016-07-26 at 21:35:05 +0200, Vadim Kochan  wrote:
>> Implemented 'dinc' and 'drnd' functions to be used for proto fields,
>> and generate values at runtime. Parsing of proto field values
>> for unified to make extending of field functions more easier w/o
>> copy/paste similar rules for each proto field. Instead of that
>> the field_expr struct is used to keep specified field and value which
>> might be a func in the following form:
>>
>> (=())
>>
>> Fields which has dynamic functions are stored in packet_dyn structure and
>> generated after each packet run. After fields are updated the L3/L4 headers
>> updates its csum if it is needed.
>>
>> Changed field lookup logic to make field set/get operations a little bit 
>> faster by
>> using field id as index in the array, as usually fields are defined regarding
>> its index.
>>
>> Example:
>>
>> { eth(sa=11:22:33:44:55:66, sa=dinc()), udp(sp=drnd(), dp=drnd()) }
>>
>> Vadim Kochan (15):
>>   trafgen: Move applying dynamic elements to function
>>   trafgen: proto: Reference to packet from proto_hdr struct
>>   trafgen: proto: Move proto headers into packet
>>   trafgen: proto: Force field id to be index in array
>>   trafgen: proto: Increment proto field at runtime
>>   trafgen: proto: Randomize proto field at runtime
>>   trafgen: ipv4: Update csum at runtime if needed
>>   trafgen: icmpv4: Update csum at runtime if needed
>>   trafgen: icmpv6: Update csum at runtime if needed
>>   trafgen: udp: Update csum at runtime if needed
>>   trafgen: tcp: Update csum at runtime if it needed
>>   trafgen: parser: Unify proto field value parsing
>>   trafgen: parser: Add support of 'dinc' function for proto fields
>>   trafgen: parser: Add 'drnd()' function for proto fields
>>   trafgen: man: Add description for 'dinc' and 'drnd' field functions
>
> While reviewing (and partially applying) this series I noticed something
> about the current implementation of trafgen protos which I find quite
> odd:
>
> The struct proto_hdr is on one hand used to statically define protocol
> behavior (i.e. all the *_hdr definitions in trafgen_l{2,3,4}.c) and
> these are then memcpy()'ed for each created packed at parse time and
> then used to store dynamically changing information at parse/run time.
> This way, members such as the proto id, layer and the pointers callback
> functions get copied for each created packet (in addition to the other
> fields which get changed during parsing) and static/dynamic information
> get mixed and we e.g. can't make the protocol definitions const to
> ensure they'll not get changed.
>
> Rather than copying the struct proto_hdr for every packet, I think it
> would be cleaner to separate it into two structs, one for the statically
> defined protocol (let's call it struct proto_ops) and one for the
> dynamic information (this would be the 'new', reduced struct proto_hdr).
> The struct proto_hdr would then have a pointer to the corresponding
> proto_ops instance and could use it to execute callbacks. The new
> structs would look something like this:
>
> struct proto_ops {
> enum proto_id id;
> enum proto_layer layer;
>
> void (*header_init)(struct proto_hdr *hdr);
> void (*header_finish)(struct proto_hdr *hdr);
> void (*packet_finish)(struct proto_hdr *hdr);
> void (*set_next_proto)(struct proto_hdr *hdr, enum proto_id pid);
> };
>
> struct proto_hdr {
> struct proto_ops *ops;
> uint16_t pkt_offset;
> uint32_t pkt_id;
> struct proto_field *fields;
> size_t fields_count;
> size_t len;
> };
>
> Maybe I missed something important, why this wouldn't work or isn't a
> good idea? But if not, and if you agree I'd like to make this change
> before the reroll of your series.
>
> If that's OK, I'll prepare the series and send it as an RFC to you,
> Daniel and the list for review.
>
> --
> 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.

Hi Tobias,

proto_ops sounds good, *BUT* would not it better to do this
refactoring ( I can do this by myself ) after applying this v2 series
(or vX if needed),
really it will save me from conflicts fighting :), of course I dont
think the conflicts will be serious but because of this
series is quite big - I'd like to focus manly on fixes which are
related to this series, if you dont mind.

Regarding why I used one proto_hdr to keep it all here, well I did not
take it into account, and I was mostly focused on if it is possible to
implement it and how it will work and if it can be simple as possible
and +/- to make able to create some generic API,
so there was no some serious reason, 

[netsniff-ng] Re: [PATCH 00/15] trafgen: Support dinc & drnd for proto fields

[Tobias Klauser]

Hi Vadim

On 2016-07-26 at 21:35:05 +0200, Vadim Kochan  wrote:
> Implemented 'dinc' and 'drnd' functions to be used for proto fields,
> and generate values at runtime. Parsing of proto field values
> for unified to make extending of field functions more easier w/o
> copy/paste similar rules for each proto field. Instead of that
> the field_expr struct is used to keep specified field and value which
> might be a func in the following form:
> 
> (=())
> 
> Fields which has dynamic functions are stored in packet_dyn structure and
> generated after each packet run. After fields are updated the L3/L4 headers
> updates its csum if it is needed.
> 
> Changed field lookup logic to make field set/get operations a little bit 
> faster by
> using field id as index in the array, as usually fields are defined regarding
> its index.
> 
> Example:
> 
> { eth(sa=11:22:33:44:55:66, sa=dinc()), udp(sp=drnd(), dp=drnd()) }
> 
> Vadim Kochan (15):
>   trafgen: Move applying dynamic elements to function
>   trafgen: proto: Reference to packet from proto_hdr struct
>   trafgen: proto: Move proto headers into packet
>   trafgen: proto: Force field id to be index in array
>   trafgen: proto: Increment proto field at runtime
>   trafgen: proto: Randomize proto field at runtime
>   trafgen: ipv4: Update csum at runtime if needed
>   trafgen: icmpv4: Update csum at runtime if needed
>   trafgen: icmpv6: Update csum at runtime if needed
>   trafgen: udp: Update csum at runtime if needed
>   trafgen: tcp: Update csum at runtime if it needed
>   trafgen: parser: Unify proto field value parsing
>   trafgen: parser: Add support of 'dinc' function for proto fields
>   trafgen: parser: Add 'drnd()' function for proto fields
>   trafgen: man: Add description for 'dinc' and 'drnd' field functions

While reviewing (and partially applying) this series I noticed something
about the current implementation of trafgen protos which I find quite
odd:

The struct proto_hdr is on one hand used to statically define protocol
behavior (i.e. all the *_hdr definitions in trafgen_l{2,3,4}.c) and
these are then memcpy()'ed for each created packed at parse time and
then used to store dynamically changing information at parse/run time.
This way, members such as the proto id, layer and the pointers callback
functions get copied for each created packet (in addition to the other
fields which get changed during parsing) and static/dynamic information
get mixed and we e.g. can't make the protocol definitions const to
ensure they'll not get changed.

Rather than copying the struct proto_hdr for every packet, I think it
would be cleaner to separate it into two structs, one for the statically
defined protocol (let's call it struct proto_ops) and one for the
dynamic information (this would be the 'new', reduced struct proto_hdr).
The struct proto_hdr would then have a pointer to the corresponding
proto_ops instance and could use it to execute callbacks. The new
structs would look something like this:

struct proto_ops {
enum proto_id id;
enum proto_layer layer;

void (*header_init)(struct proto_hdr *hdr);
void (*header_finish)(struct proto_hdr *hdr);
void (*packet_finish)(struct proto_hdr *hdr);
void (*set_next_proto)(struct proto_hdr *hdr, enum proto_id pid);
};

struct proto_hdr {
struct proto_ops *ops;
uint16_t pkt_offset;
uint32_t pkt_id;
struct proto_field *fields;
size_t fields_count;
size_t len;
};

Maybe I missed something important, why this wouldn't work or isn't a
good idea? But if not, and if you agree I'd like to make this change
before the reroll of your series.

If that's OK, I'll prepare the series and send it as an RFC to you,
Daniel and the list for review.

-- 
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 04/15] trafgen: proto: Force field id to be index in array

[Vadim Kochan]

OK, actually I fixed it in my v2 branch ...

On Tue, Aug 9, 2016 at 2:59 PM, Tobias Klauser  wrote:
> On 2016-07-26 at 21:35:09 +0200, Vadim Kochan  wrote:
>> Usually proto fields array is sorted in the same order as related enum,
>> so id may be used as index for faster lookup, it will make
>> csum field calculation little faster at runtime.
>>
>> Signed-off-by: Vadim Kochan 
>
> I now applied the patch (with added comment and bug_on() check) now and
> as a follow-up also did the same thing for the 'registered' array, see
> current HEAD.

-- 
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 04/15] trafgen: proto: Force field id to be index in array

[Tobias Klauser]

On 2016-07-26 at 21:35:09 +0200, Vadim Kochan  wrote:
> Usually proto fields array is sorted in the same order as related enum,
> so id may be used as index for faster lookup, it will make
> csum field calculation little faster at runtime.
> 
> Signed-off-by: Vadim Kochan 

I now applied the patch (with added comment and bug_on() check) now and
as a follow-up also did the same thing for the 'registered' array, see
current HEAD.

-- 
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 05/15] trafgen: proto: Increment proto field at runtime

[Vadim Kochan]

On Tue, Aug 9, 2016 at 9:57 AM, Tobias Klauser  wrote:
> On 2016-08-08 at 21:04:20 +0200, Vadim Kochan  wrote:
>> On Sat, Aug 06, 2016 at 01:36:26AM +0300, Vadim Kochan wrote:
>> > On Wed, Aug 03, 2016 at 04:27:14PM +0200, Tobias Klauser wrote:
>> > > On 2016-07-26 at 21:35:10 +0200, Vadim Kochan  wrote:
>> > > > Extended 'struct packet_dyn' with proto fields which has
>> > > > dynamically changing values at runtime.
>> > > >
>> > > > Implement incrementing of proto field at runtime with min & max
>> > > > parameters, by default if the 'min' parameter is not specified
>> > > > then original value is used. For fields which len is greater
>> > > > than 4 - last 4 bytes are incremented as unsigned int value.
>> > > >
>> > > > Added 'field_changed' callback for proto header which
>> > > > may be used for check if csum updating is needed. This callback
>> > > > is called after field was changed at runtime.
>> > > >
>> > > > Added 'packet_update' callback to let proto header know
>> > > > when to apply final proto header changes at runtime (csum update).
>> > >
>> > > The documentation of these callbacks would also make sense where they're
>> > > defined.
>> > >
>> > > > Signed-off-by: Vadim Kochan 
>> > > > ---
>> > > >  trafgen.c   |  9 ++
>> > > >  trafgen_conf.h  |  7 
>> > > >  trafgen_proto.c | 99 
>> > > > +
>> > > >  trafgen_proto.h | 26 +++
>> > > >  4 files changed, 141 insertions(+)
>> > > >
>> > > > diff --git a/trafgen.c b/trafgen.c
>> > > > index b76b5d7..553dfa5 100644
>> > > > --- a/trafgen.c
>> > > > +++ b/trafgen.c
>> > > >  void proto_packet_finish(void)
>> > > >  {
>> > > > struct proto_hdr **headers = _packet()->headers[0];
>> > > > @@ -433,3 +446,89 @@ void proto_packet_finish(void)
>> > > > p->packet_finish(p);
>> > > > }
>> > > >  }
>> > > > +
>> > > > +static inline unsigned int field_inc(struct proto_field *field)
>> > > > +{
>> > > > +   uint32_t val;
>> > > > +
>> > > > +   val = field->func.val - field->func.min;
>> > > > +   val = (val + field->func.inc) % field->func.max;
>> > >
>> > > Shouldn't this be
>> > >
>> > >   val = (val + field->func.inc) % (field->func.max - field->func.min + 1)
>> > >
>> > > to be consistent with apply_counter()?
>>
>> I simplified it and now it really works well:
>>
>>   #define max_int32(a, b)
>>  \
>>   ({ 
>>  \
>>   int32_t _a = (int32_t) (a);
>>  \
>>   int32_t _b = (int32_t) (b);
>>  \
>>   _a - ((_a - _b) & ((_a - _b) >> (sizeof(int32_t) * 8 - 
>> 1)));\
>
> The line above definitely needs an explanatory comment. How and why does
> this work? Why is it implemented like this instead of the "obvious"
> max() solution?

I was searching for fast max() alg, but I don't have understanding of
it, I just tested it,
I will get rid of it and use max(x).

>
>>   })
>>
>>   static inline unsigned int field_inc(struct proto_field *field)
>>   {
>>  uint32_t min = field->func.min;
>>  uint32_t max = field->func.max;
>>  uint32_t val = field->func.val;
>>  uint32_t inc = field->func.inc;
>>  uint32_t next;
>>
>>  next = (val + inc) % (max + 1);
>>  field->func.val = max_int32(next, min);
>>
>>  return val;
>>   }
>>
>> so max_int32(a,b) should be fast enough w/o branching.
>
> Have you profiled this against other possible implementations,
> especially the max_int32() implementation above? If your profiling doesn't
> show significant improvements of this max_int32() variant I'd rather
> stay with the "obvious" one, i.e. a > b ? a : b

No, I did not, I just thought that branching will slow execution anyway ...

>
>> > Sure, I tried this approach while implementing 1st version but when I
>> > used the following case:
>> >
>> > trafgen/trafgen -o lo -n 10 --cpu 1 '{ eth(type=0x800), fill(0xff, 
>> > 10), dinc(5, 20, 5) }'
>> >
>> > then interval between 5 & 20 changes very differently. But in my version
>> > it repeats from 5 till 20 (yes here is a little difference that initial
>> > value is incremented immideately). Also semantic of proto dinc is
>> > dinc(step, min, max), and I will change it to looks like low-level one -
>> > dinc(min, max, step).
>> >
>> > >
>> > > Also, I think you should probably get rid of as many pointer
>> > > dereferences as possible in these runtime functions, i.e. store max and
>> > > min in temporary variables.
>> >
>> > OK, makes sense.
>> >
>> > >
>> > > > +   field->func.val = val + field->func.min;
>> > > > +
>> > > > +   

[netsniff-ng] Re: [PATCH 05/15] trafgen: proto: Increment proto field at runtime

[Tobias Klauser]

On 2016-08-08 at 21:04:20 +0200, Vadim Kochan  wrote:
> On Sat, Aug 06, 2016 at 01:36:26AM +0300, Vadim Kochan wrote:
> > On Wed, Aug 03, 2016 at 04:27:14PM +0200, Tobias Klauser wrote:
> > > On 2016-07-26 at 21:35:10 +0200, Vadim Kochan  wrote:
> > > > Extended 'struct packet_dyn' with proto fields which has
> > > > dynamically changing values at runtime.
> > > > 
> > > > Implement incrementing of proto field at runtime with min & max
> > > > parameters, by default if the 'min' parameter is not specified
> > > > then original value is used. For fields which len is greater
> > > > than 4 - last 4 bytes are incremented as unsigned int value.
> > > > 
> > > > Added 'field_changed' callback for proto header which
> > > > may be used for check if csum updating is needed. This callback
> > > > is called after field was changed at runtime.
> > > > 
> > > > Added 'packet_update' callback to let proto header know
> > > > when to apply final proto header changes at runtime (csum update).
> > > 
> > > The documentation of these callbacks would also make sense where they're
> > > defined.
> > > 
> > > > Signed-off-by: Vadim Kochan 
> > > > ---
> > > >  trafgen.c   |  9 ++
> > > >  trafgen_conf.h  |  7 
> > > >  trafgen_proto.c | 99 
> > > > +
> > > >  trafgen_proto.h | 26 +++
> > > >  4 files changed, 141 insertions(+)
> > > > 
> > > > diff --git a/trafgen.c b/trafgen.c
> > > > index b76b5d7..553dfa5 100644
> > > > --- a/trafgen.c
> > > > +++ b/trafgen.c
> > > >  void proto_packet_finish(void)
> > > >  {
> > > > struct proto_hdr **headers = _packet()->headers[0];
> > > > @@ -433,3 +446,89 @@ void proto_packet_finish(void)
> > > > p->packet_finish(p);
> > > > }
> > > >  }
> > > > +
> > > > +static inline unsigned int field_inc(struct proto_field *field)
> > > > +{
> > > > +   uint32_t val;
> > > > +
> > > > +   val = field->func.val - field->func.min;
> > > > +   val = (val + field->func.inc) % field->func.max;
> > > 
> > > Shouldn't this be
> > > 
> > >   val = (val + field->func.inc) % (field->func.max - field->func.min + 1)
> > > 
> > > to be consistent with apply_counter()?
> 
> I simplified it and now it really works well:
> 
>   #define max_int32(a, b) 
> \
>   ({  
> \
>   int32_t _a = (int32_t) (a); 
> \
>   int32_t _b = (int32_t) (b); 
> \
>   _a - ((_a - _b) & ((_a - _b) >> (sizeof(int32_t) * 8 - 
> 1)));\

The line above definitely needs an explanatory comment. How and why does
this work? Why is it implemented like this instead of the "obvious"
max() solution?

>   })
> 
>   static inline unsigned int field_inc(struct proto_field *field)
>   {
>  uint32_t min = field->func.min;
>  uint32_t max = field->func.max;
>  uint32_t val = field->func.val;
>  uint32_t inc = field->func.inc;
>  uint32_t next;
> 
>  next = (val + inc) % (max + 1);
>  field->func.val = max_int32(next, min);
> 
>  return val;
>   }
> 
> so max_int32(a,b) should be fast enough w/o branching.

Have you profiled this against other possible implementations,
especially the max_int32() implementation above? If your profiling doesn't
show significant improvements of this max_int32() variant I'd rather
stay with the "obvious" one, i.e. a > b ? a : b

> > Sure, I tried this approach while implementing 1st version but when I
> > used the following case:
> > 
> > trafgen/trafgen -o lo -n 10 --cpu 1 '{ eth(type=0x800), fill(0xff, 10), 
> > dinc(5, 20, 5) }'
> > 
> > then interval between 5 & 20 changes very differently. But in my version
> > it repeats from 5 till 20 (yes here is a little difference that initial
> > value is incremented immideately). Also semantic of proto dinc is
> > dinc(step, min, max), and I will change it to looks like low-level one -
> > dinc(min, max, step).
> > 
> > > 
> > > Also, I think you should probably get rid of as many pointer
> > > dereferences as possible in these runtime functions, i.e. store max and
> > > min in temporary variables.
> > 
> > OK, makes sense.
> > 
> > > 
> > > > +   field->func.val = val + field->func.min;
> > > > +
> > > > +   return field->func.val;
> > > > +}
> > > > +
> > > > +static void field_inc_func(struct proto_field *field)
> > > > +{
> > > > +   if (field->len == 1) {
> > > > +   uint8_t val;
> > > > +
> > > > +   val = field_inc(field);
> > > > +   proto_field_set_u8(field->hdr, field->id, val);
> > > 
> > > Assignment on declaration please. Or even better:
> 

[netsniff-ng] Re: [PATCH v3 3/3] configure: Add option to compile tools without libnl dependency

[Tobias Klauser]

On 2016-08-08 at 16:31:11 +0200, Vadim Kochan  wrote:
> On Mon, Aug 8, 2016 at 5:24 PM, Tobias Klauser  wrote:
> > On 2016-08-06 at 11:59:05 +0200, Vadim Kochan  wrote:
> >> On Sat, Aug 6, 2016 at 12:41 PM, Tobias Klauser  
> >> wrote:
> >> > On 2016-08-05 at 22:51:54 +0200, Vadim Kochan  wrote:
> >> Yes, I understand, I said so because I really remember we talked about
> >> disabling of libnl support earier ago and you or Daniel pointed that
> >> it is better to have this dependency by default, this is how I
> >> remember this, I might be wrong, but anyway
> >> it simplifies build netsniff-ng/trafgen manually w/o searching libnl-
> >> packages on specific dist.
> >
> > I can't remember we talked about it, but that doesn't mean we didn't :)
> > In any case if we discussed a particular feature/patch/RFC/... before
> > and you patch is based on the previous discussion, please include a link
> > to the respective mailing list thread into the description. This may
> > help to remind us of past conversations ;)
> >
> > Thanks!
> 
> This is a link with our discussion (within libnl-route fix titled with
> "[PATCH] build: Check for libnl-route"), but I looked for this
> just now so I did not consider it while writing previous email, so I
> might be wrong for some conclusions):
> 
> https://groups.google.com/forum/?hl=en#!searchin/netsniff-ng/Check$20for$20libnl-route%7Csort:relevance/netsniff-ng/0Ws7ecy1H_Y/DIzprE4KBAAJ

Thanks for the link. I don't see detailled discussion of how this should
be implemented, just the general idea. And there, I think, we all agreed
that it would be fine as long as it doesn't introduce too much
additional complexity.

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