[ovs-dev] [PATCH 4/4] ofproto: Implement IPFIX exporter

Romain Lenglet rlenglet at vmware.com
Thu Jan 10 23:37:37 UTC 2013


On Jan 9, 2013, at 4:00 PM, Ben Pfaff <blp at nicira.com> wrote:

> ipfix-gen-entities is only used at build time, that is, we do not
> install it, so I do not think it is worthwhile to generate it from a
> .in file.  (For Python programs, that is mostly to get the right
> Python binary, which isn't necessary at build time since we just run
> it explicitly.  The @VERSION@ substitution isn't very useful at build
> time either, since the build tool is never installed.)

I agree. I've moved it into ofproto_libofproto_a_SOURCES and added a comment, and removed it from AC_CONFIG_FILES.

[...]

>> + * By default, the interval is 10 minutes.  Here we use instead an
>> + * interval in terms of messages.  Since we send many messages (one
>> + * per sampled packet) 10000 seems reasonable and should be lower than
>> + * 10 minutes.  Cf. IETF RFC 5101 Section 10.3.6. */
>> +#define IPFIX_TEMPLATE_INTERVAL 10000
> 
> It would be easy to keep track of the time when a template record was
> most recently sent, and then send one if it was 10 minutes ago.  I
> don't know whether this is important.

You're right. It was trivial, so I did that, and updated the comment.

>> +/* Cf. IETF RFC 5101 Section 3.1. */
>> +struct ipfix_header {
>> +    uint16_t version;  /* IPFIX_VERSION. */
>> +    uint16_t length;  /* Length in bytes including this header. */
>> +    uint32_t export_time;  /* Seconds since the epoch. */
>> +    uint32_t seq_number;  /* Message sequence number. */
>> +    uint32_t obs_domain_id;  /* Observation Domain ID. */
>> +} __attribute__((packed));
> 
> On many RISC architectures, access to "packed" data structures is more
> expensive than access to unpacked data structures.  So, in OVS, we try
> to use "packed" only if a data structure's natural packing would
> differ from the packed format, or if we expect the data structure to
> be accessed when it is misaligned.  The former doesn't look true here.
> I didn't read the code carefully enough to figure out the latter, but
> it is rarely true.  So I would be inclined to omit the "packed"
> attribute here.

As you commented in your other review, IPFIX allows for some entities to be unaligned, of variable length or 1-byte long, and makes padding optional, so I'd feel safer to keep the packed attribute even if it may not be strictly necessary right now.

[...]

> ipfix_init_header() only uses the tv_sec part of the timespec returned
> by time_wall_timespec(), so it could just use time_wall() directly.

Done.

> It looks like, in practice, the arguments to
> ipfix_define_template_entity() are always fixed at compile time.  If
> I'm reading the code correctly, then this means that each call to
> ipfix_define_template_entity() is going through a "switch" statement
> to find out a couple of values whose values could actually be fixed at
> compile time.  So, how about something like this, instead:
> 
>   - at top level, add:
> 
> enum ipfix_entity_size {
> #define IPFIX_ENTITY(ENUM, ID, SIZE, NAME)  IPFIX_ENTITY_SIZE_##ENUM,
> #include "ofproto/ipfix-entities.def"
> };
> 
>   - Change ipfix_define_template_entity() to just:
> 
> static void
> ipfix_define_template_entity(enum ipfix_entity_id id, int size,
>                             struct ofpbuf *msg)
> {
>    struct ipfix_template_field_specifier *field;
> 
>    field = ofpbuf_put_zeros(msg, sizeof *field);
>    field->element_id = htons(id);
>    field->field_length = htons(size);
> }
> 
>   - Change DEF to just:
> 
> #define DEF(ID)                                                         \
>    {                                                                   \
>        ipfix_define_template_entity(IPFIX_ENTITY_ID_##ID,              \
>                                     IPFIX_ENTITY_SIZE_##ID, msg);      \
>        count++;                                                        \
>    }
> 
> and thereby avoid runtime work for what we know at compile time.

How would that work?
Nothing uses the SIZE argument of the IPFIX_ENTITY in your code above, and you're using the enum value in place of the size, which seems incorrect.

> 
>> +    /* TODO: Extract additional headers from the packet when not in
>> +     * struct flow:
>> +     * ETHERNET_HEADER_LENGTH
>> +     * ETHERNET_PAYLOAD_LENGTH
>> +     * ETHERNET_TOTAL_LENGTH */
> 
> The above would be pretty easy to extract, if they are useful.
> 
>> +        /* TODO: Extract additional headers from the packet when not
>> +         * in struct flow:
>> +         * IP_HEADER_LENGTH
>> +         * TOTAL_LENGTH_IPV4
>> +         * IP_TOTAL_LENGTH
>> +         * PAYLOAD_LENGTH_IPV6 */
> 
> As would these, I think, with the possible exception of IPv6 (for
> which nothing is ever easy.)

Right. I added code to calculate those based on the l2, l3, l4 fields of ofpbuf.

[...]

> I have to read the rest of the code, but I thought I'd send this out
> for an initial review

Thanks for your comments!
I addressed all, and will send updated patches after reviewing your last email.


More information about the dev mailing list