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

[Vadim Kochan]

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

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.

> 
> 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:
> > 
> > proto_field_set_u8(field->hdr, field->id, field_inc(field));
> 
> OK
> 
> > 
> > > + } else if (field->len == 2) {
> > > + uint16_t val;
> > > +
> > > + val = field_inc(field);
> > > + proto_field_set_be16(field->hdr, field->id, val);
> > 
> > Same.
> OK
> 
> > 
> > > + } else if (field->len == 4) {
> > > + uint32_t val;
> > > +
> > > + val = field_inc(field);
> > > + proto_field_set_be32(field->hdr, field->id, val);
> > 
> > Same.
> OK
> 
> > 
> > > + } else if (field->len > 4) {
> > > + uint8_t *bytes = __proto_field_get_bytes(field);
> > > + uint32_t val;
> > > +
> > > + bytes += field->len - 4;
> > > + val = field_inc(field);
> > > +
> > > +

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

[Vadim Kochan]

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:
>> >> Hm,
>> >>
>> >> My version was to keep current strict dependency on libnl by default,
>> >> so if there is no libnl then
>> >> netsniff-ng & trafgen will be removed from build list, but make
>> >> possible to skip this
>> >> rule by --disable-libnl option. Now I am not sure there is a reason
>> >> for --disable-libnl as if there is no libnl
>> >> then netsniff-ng & trafgen will be added to build list, and will be
>> >> compiled w/o libnl dependency..
>> >
>> > Ok, but that intention of your patch wasn't really clear from the
>> > description, sorry.
>>
>> Yes, usually I put no so much description into patches.
>
> Please try to be as verbose as possible in patch descriptions (at least
> for non-trivial patches). The description is there to help others
> understand the rationale behind a patch (before applying it and
> especially afterwards e.g. when chasing bugs).
>
>> >> So does --disable-libnl makes sense ?
>> >
>> > If you have libnl-dev installed but want to compile netsniff-ng/trafgen
>> > without libnl support it still makes sense.
>> >
>> > IMO the current version is more in line with the behavior we already
>> > have for geoip and libz.
>> >
>> > Thanks
>> > Tobias
>>
>> 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

-- 
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 v3 3/3] configure: Add option to compile tools without libnl dependency

[Tobias Klauser]

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:
> >> Hm,
> >>
> >> My version was to keep current strict dependency on libnl by default,
> >> so if there is no libnl then
> >> netsniff-ng & trafgen will be removed from build list, but make
> >> possible to skip this
> >> rule by --disable-libnl option. Now I am not sure there is a reason
> >> for --disable-libnl as if there is no libnl
> >> then netsniff-ng & trafgen will be added to build list, and will be
> >> compiled w/o libnl dependency..
> >
> > Ok, but that intention of your patch wasn't really clear from the
> > description, sorry.
> 
> Yes, usually I put no so much description into patches.

Please try to be as verbose as possible in patch descriptions (at least
for non-trivial patches). The description is there to help others
understand the rationale behind a patch (before applying it and
especially afterwards e.g. when chasing bugs).

> >> So does --disable-libnl makes sense ?
> >
> > If you have libnl-dev installed but want to compile netsniff-ng/trafgen
> > without libnl support it still makes sense.
> >
> > IMO the current version is more in line with the behavior we already
> > have for geoip and libz.
> >
> > Thanks
> > Tobias
> 
> 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!

-- 
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: trafgen: Would be it useful to have pktgen support ?

[Daniel Borkmann]

On 08/07/2016 07:46 PM, Vadim Kochan wrote:

Hi,

I did not ever use Linux pktgen feature, but I just catch the
idea if it would be good to have option to send trafgen protocol
built packet via Linux pktgen ? Theoretically it is possible to create
simple and generic code to generate raw or pktgen packets. But
pktgen's kind of packets will be possible to generate only via protocol
header functions because with it we have all field metadata. Also
when dynamic fields series will be applied it may be possible also
convert them to pktgen's rand/inc commands.

But again let me know if it might be useful.


I think it would be useful, I see value where trafgen would do all the
procfs setup details internally and then runs pktgen as a backend. So the
only thing needed for the user would be to execute trafgen normally via
cmdline.

You won't be able to use all of the trafgen features when selecting pktgen
instead of af_packet as a backend, but I think that's okay.

Thanks,
Daniel

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