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

[Ben Pfaff]

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 <dani...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev