[ovs-dev] [PATCH] Genericize/simplify kernel sFlow implementation

[Jesse Gross]

On Tue, Sep 13, 2011 at 10:55 AM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Mon, Sep 12, 2011 at 4:37 PM, Jesse Gross <jesse at nicira.com> wrote:
>> On Tue, Aug 30, 2011 at 6:59 PM, Pravin Shelar <pshelar at nicira.com> wrote:
>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>> index 8aec438..3178bdd 100644
>>> --- a/datapath/actions.c
>>> +++ b/datapath/actions.c
>>> +static int sample(struct datapath *dp, struct sk_buff *skb,
>>> +                 const struct nlattr *a)
>>> +{
>>> +       u32 probability;
>>> +       const struct nlattr *sample_args, *nested_act;
>>> +       int rem, err;
>>> +
>>> +       rem = nla_len(a);
>>> +
>>> +       /* First arg: probability */
>>
>> It's not really good to be mandating specific positioning of the
>> attributes since it makes future compatibility more difficult.  This
>> is perhaps an exception because it is on the fast path (another good
>> reason to not do Netlink parsing here).  On the other hand, maybe it's
>> better to do it right and then we can optimize it in the future.  In
>> any case, this function currently has a strange duality between caring
>> about performance or not.  For example, nla_for_each_nested() has
>> extra checks that aren't necessary because we already did validation.
>>
> I am not sure about future compatibility. Any sampling action must
> have probability argument. So how does it make it difficult? do u have
> an example of that?

It's not for the probability argument, it's for any new ones that we
might want to add in the future.  A looser coupling allows new
arguments to be added and skipped if not recognized.  Perhaps more
importantly, it goes against the Netlink convention and makes some
things difficult as you can't use the normal parsing functions.

>>> +       /* Second arg: actions to execute */
>>> +       sample_args = nla_next(sample_args, &rem);
>>> +       /* Second arg data contains actions to execute */
>>> +       nla_for_each_nested(nested_act, sample_args, rem) {
>>> +               switch (nla_type(nested_act)) {
>>> +                       case OVS_ACTION_ATTR_USERSPACE:
>>> +                               err = output_userspace(dp, skb,
>>> +                                                nla_get_u64(nested_act));
>>> +                               return err;
>>> +                       break;
>>> +                       default:
>>> +                               return -EINVAL;
>>> +               }
>>> +       }
>>
>> I think that it's reasonable to restrict the supported actions
>> initially but it's not clear to me that it's actually a win either
>> here or any of the other places that this is now scattered around the
>> code base.  Since handling the new action list is simply a recursive
>> call, I think it's actually pretty straightforward to support all
>> actions without any special casing.
>>
> I can not test arbitrary set of actions that why I restricted that
> set. any ways we can allow all
> actions except SAMPLE as arg.

I think it's even OK to allow recursive sample within limits to
prevent blowing out the kernel stack.