[ovs-dev] [ovs-dev, PATCHv3] ofproto-dpif-sflow: Add snaplen for sample action and sFlow.

William Tu u9012063 at gmail.com
Mon Aug 1 16:59:54 UTC 2016


Thanks. I agree that we should remove IPFIX and sample action support.
I will send v4 for review.

Regards,
William

On Fri, Jul 22, 2016 at 2:38 PM, Ben Pfaff <blp at ovn.org> wrote:
> It sounds like snaplen is not very useful in IPFIX.  If so, then I'd
> support removing the IPFIX and sample action support for it.
>
> If you agree, will you send a v4?  Otherwise, let me know and I'll apply
> v3.
>
> Here's a change I'd like to fold into v3, to better allow for future
> extensions.  It matches the way that we treat padding fields in most
> actions these days.
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index a3a14b0..cd528a4 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -4779,7 +4779,7 @@ struct nx_action_sample2 {
>      ovs_be32 obs_point_id;          /* ID of sampling observation point. */
>      ovs_be16 sampling_port;         /* Sampling port. */
>      ovs_be16 snaplen;               /* Max sampled packet size in byte. */
> -    uint8_t  pad[4];                /* Pad to a multiple of 8 bytes */
> +    uint8_t  zeros[4];              /* Pad to a multiple of 8 bytes */
>   };
>   OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
>
> @@ -4812,6 +4812,10 @@ decode_NXAST_RAW_SAMPLE2(const struct nx_action_sample2 *nas,
>                           enum ofp_version ofp_version OVS_UNUSED,
>                           struct ofpbuf *out)
>  {
> +    if (!is_all_zeros(nas->zeros, sizeof nas->zeros)) {
> +        return OFPERR_NXBRC_MUST_BE_ZERO;
> +    }
> +
>      struct ofpact_sample *sample;
>
>      sample = ofpact_put_SAMPLE(out);
>
>
> Thanks,
>
> Ben.
>
> On Sun, Jul 10, 2016 at 09:24:45PM -0700, William Tu wrote:
>> Hi Daniel,
>>
>> Thanks for reviewing the patch.
>> Indeed, the way sFlow sets up the datapath does not require the
>> OpenFlow sample action, and changing OVSDB/compose_sample_action() is
>> sufficient to program the datapath sample action for current sFlow use
>> case. So if IPFIX does not use 'snaplen', or there is no future use
>> case, then I could remove the snaplen in "struct nx_action_sample2".
>>
>> Regards,
>> William
>>
>> On Sun, Jul 10, 2016 at 7:46 PM, Daniel Ye <daniely at vmware.com> wrote:
>> > Hi William,
>> >
>> >
>> >
>> > I saw that you submitted the patch below. It’s about adding ‘snaplen’ for
>> > sample action and sFlow.
>> >
>> > https://patchwork.ozlabs.org/patch/645013/
>> >
>> >
>> >
>> > I’m still confused whether sFlow uses the openflow sample action. In my
>> > opinion, sFlow only uses the datapath
>> >
>> > sample action, not the sample action in userworld. Actually, there are two
>> > kinds of IPFIX, one is bridge level IPFIX and the
>> >
>> > other is flow-based IPFIX. Bridge level IPFIX doesn’t use openflow sample
>> > action, it only uses the datapath sample action. I think
>> >
>> > sFlow does the same as bridge level IPFIX. For bridge level IPFIX and sFlow,
>> > all the parameters are stored in OVSDB.
>> >
>> >
>> >
>> > Actually, “nx_action_sample2” is only used by flow-based IPFIX now. If you
>> > add a ‘snaplen’ field in it, I don’t think this field
>> >
>> > will be used by sFlow. Maybe you only need to change
>> > “compose_sample_action()” and put ‘snaplen’ in OVSDB??
>> >
>> >
>> >
>> > For all the alias, does any sFlow experts come to make sure of this?
>> >
>> >
>> >
>> > Thanks,
>> >
>> > Daniel Benli Ye



More information about the dev mailing list