On Sat, Mar 18, 2017 at 12:22 PM, Pravin Shelar <pshe...@ovn.org> wrote:
> On Thu, Mar 16, 2017 at 3:48 PM, Andy Zhou <az...@ovn.org> wrote:
>> Added clone_execute() that both the sample and the recirc
>> action implementation can use.
>>
>> Signed-off-by: Andy Zhou <az...@ovn.org>
>> ---
>> net/openvswitch/actions.c | 175
>> ++++++++++++++++++++++++----------------------
>> 1 file changed, 92 insertions(+), 83 deletions(-)
>>
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 3529f7b..e38fa7f 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -44,10 +44,6 @@
>> #include "conntrack.h"
>> #include "vport.h"
>>
>> -static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> - struct sw_flow_key *key,
>> - const struct nlattr *attr, int len);
>> -
>> struct deferred_action {
>> struct sk_buff *skb;
>> const struct nlattr *actions;
>> @@ -166,6 +162,12 @@ static bool is_flow_key_valid(const struct sw_flow_key
>> *key)
>> return !(key->mac_proto & SW_FLOW_KEY_INVALID);
>> }
>>
>> +static int clone_execute(struct datapath *dp, struct sk_buff *skb,
>> + struct sw_flow_key *key,
>> + u32 recirc_id,
>> + const struct nlattr *actions, int len,
>> + bool last, bool clone_flow_key);
>> +
> With this function the diff stat looks much better.
>
> ...
> ...
>> +/* Execute the actions on the clone of the packet. The effect of the
>> + * execution does not affect the original 'skb' nor the original 'key'.
>> + *
>> + * The execution may be deferred in case the actions can not be executed
>> + * immediately.
>> + */
>> +static int clone_execute(struct datapath *dp, struct sk_buff *skb,
>> + struct sw_flow_key *key, u32 recirc_id,
>> + const struct nlattr *actions, int len,
>> + bool last, bool clone_flow_key)
>> +{
>> + bool is_sample = actions;
> Standard practice is use !! to convert pointer to boolean.
> I think this function does not need to know about sample action. So we
> can rename the boolean to have_actions or something similar.
O.K. We can just check for actions pointer.
However, it will be obvious we are actually checking for sample or recirc
action. I will add some comments.
>
>> + struct deferred_action *da;
>> + struct sw_flow_key *clone;
>> + int err = 0;
>> +
>> + skb = last ? skb : skb_clone(skb, GFP_ATOMIC);
>> + if (!skb) {
>> + /* Out of memory, skip this action.
>> + */
>> + return 0;
>> + }
>> +
>> + /* In case the sample actions won't change the 'key',
>> + * current key can be used directly to execute sample actions.
>> + * Otherwise, allocate a new key from the
>> + * next recursion level of 'flow_keys'. If
>> + * successful, execute the sample actions without
>> + * deferring.
>> + */
>> + if (is_sample && clone_flow_key)
>> + __this_cpu_inc(exec_actions_level);
>> +
> There is no need to increment actions level up here. it is only
> required for do_execute_actions(). ovs_dp_process_packet() already
> does it.
Right, that's why it is only done for 'is_sample'. There is a bug though,
the 'inc' needs to be done after clone. I will rearrange the code to move 'inc'
only above the do_execute_actions().
>
>
>> + clone = clone_flow_key ? clone_key(key) : key;
>> + if (clone) {
>> + if (is_sample) {
>> + err = do_execute_actions(dp, skb, clone,
>> + actions, len);
>> + } else {
>> + clone->recirc_id = recirc_id;
>> + ovs_dp_process_packet(skb, clone);
>> + }
>> + }
> wont this execute action twice, once here and again in deferred actions list?
Right, the return is missing for the if (clone) case. I will post v4
soon. Thanks for the review
and comments.
>
>> +
>> + if (is_sample && clone_flow_key)
>> + __this_cpu_dec(exec_actions_level);
>> +
>> + /* Out of 'flow_keys' space. Defer them */
>> + da = add_deferred_actions(skb, key, actions, len);
>> + if (da) {
>> + if (!is_sample) {
>> + key = &da->pkt_key;
>> + key->recirc_id = recirc_id;
>> + }
>> + } else {
>> + /* Drop the SKB and log an error. */
>> + kfree_skb(skb);
>> +
>> + if (net_ratelimit()) {
>> + if (is_sample) {
>> + pr_warn("%s: deferred action limit reached,
>> drop sample action\n",
>> + ovs_dp_name(dp));
>> + } else {
>> + pr_warn("%s: deferred action limit reached,
>> drop recirc action\n",
>> + ovs_dp_name(dp));
>> + }
>> + }
>> + }
>> +
>> + return err;
>> +}
>> +
>> static void process_deferred_actions(struct datapath *dp)
>> {
>> struct action_fifo *fifo = this_cpu_ptr(action_fifos);
>> --
>> 1.8.3.1
>>
