[ovs-dev] [PATCH] Initial implementation of sFlow.

Ben Pfaff blp at nicira.com
Sat Jan 9 03:50:16 UTC 2010


On Thu, Jan 07, 2010 at 01:21:46PM -0800, Justin Pettit wrote:
> A few comments inline:
> 
> On Jan 4, 2010, at 1:24 PM, Ben Pfaff wrote:
> 
> > -(e.g. NetFlow, RSPAN, ERSPAN, IOS-like CLI), and opens the forwarding
> > -functions to programmatic extension and control.
> > +(e.g. NetFlow, sFlow, RSPAN, ERSPAN, IOS-like CLI), and opens the
> 
> According to the license, I think this should be "sFlow(R)".  I don't
> think we need to do it throughout the source code...probably just in
> these high-level "documents".

OK, I made that change here, and in the manpages just to be sure.

> > -    * Visibility into inter-VM communication via NetFlow, SPAN, and RSPAN
> > +    * Visibility into inter-VM communication via NetFlow, sFlow, SPAN,
> 
> Same here.

Neil said in email (I think) that we only need (R) for the first
occurrence, so I left this one alone.

> > +	u64 sflow_pool;		/* Packets that could have been sampled. */
> 
> I think the sFlow definition for this is only 32-bit.  I'm guessing
> this is only increasing, since it's never reset any place.  I'm a bit
> surprised that the counters in sFlow are 32 bit in that case...

I can see why you'd want it to be 32 bits to save space in a protocol
definition, but I don't see too much value in forcing it to be 32 bits
here.  32-bit counters just cause trouble in my experience.  We still
need to figure out a workaround for the annoying 32-bit interface packet
and byte counters on 32-bit Linux.

Unless you feel strongly about it, I'd rather leave it as-is.

> By the way, the description in the comment was a bit confusing to me.
> The sFlow library is only slightly better, but I do think it helps:
> 
> 	Total number of packets that could have been sampled (i.e., packets
>         skipped by sampling process + total number of samples) 

OK, how is this as a revised structure:

/**
 * struct dp_stats_percpu - per-cpu packet processing statistics for a given
 * datapath.
 * @n_frags: Number of IP fragments processed by datapath.
 * @n_hit: Number of received packets for which a matching flow was found in
 * the flow table.
 * @n_miss: Number of received packets that had no matching flow in the flow
 * table.  The sum of @n_hit and @n_miss is the number of packets that have
 * been received by the datapath.
 * @n_lost: Number of received packets that had no matching flow in the flow
 * table that could not be sent to userspace (normally due to an overflow in
 * one of the datapath's queues).
 * @sflow_pool: Number of packets that were candidates for sFlow sampling,
 * regardless of whether they were actually chosen and sent down to userspace.
 */
struct dp_stats_percpu {
	u64 n_frags;
	u64 n_hit;
	u64 n_missed;
	u64 n_lost;
	u64 sflow_pool;
};

I pushed it out, for what it's worth.
 
> > + * @sflow_probability: Probability of sampling a packet to the %ODPL_SFLOW
> > + * queue, where 0 means never sample, UINT_MAX means always sample, and
> > + * other values are intermediate probabilities.
> 
> The name and description might indicate to me that the probability is
> "sflow_probability / 100".  What about a parenthetical comment that
> the value is "UINT32_MAX / sflow_probability"?

I changed it to:

 * @sflow_probability: Number of packets out of UINT_MAX to sample to the
 * %ODPL_SFLOW queue, e.g. (@sflow_probability/UINT_MAX) is the probability of
 * sampling a given packet.

I hope that's better.

> > +#define ODP_SET_SFLOW_PROBABILITY _IOR('O', 20, int)
> > +#define ODP_GET_SFLOW_PROBABILITY _IOW('O', 21, int)
> 
> Is there a reason you didn't use 19 and 20?

I guess I can't count.  Fixed.

I didn't get to the rest of your email yet.  I'll take care of that on
Monday, I guess.




More information about the dev mailing list