From patchwork Tue Apr 9 07:50:27 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Horman X-Patchwork-Id: 234980 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 80A982C00C2 for ; Tue, 9 Apr 2013 17:50:35 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935258Ab3DIHub (ORCPT ); Tue, 9 Apr 2013 03:50:31 -0400 Received: from kirsty.vergenet.net ([202.4.237.240]:34392 "EHLO kirsty.vergenet.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935093Ab3DIHua (ORCPT ); Tue, 9 Apr 2013 03:50:30 -0400 Received: from ayumi.akashicho.tokyo.vergenet.net (p8120-ipbfp1001kobeminato.hyogo.ocn.ne.jp [118.10.137.120]) by kirsty.vergenet.net (Postfix) with ESMTP id F03DD25BF01; Tue, 9 Apr 2013 17:50:28 +1000 (EST) Received: by ayumi.akashicho.tokyo.vergenet.net (Postfix, from userid 7100) id 96B63EDEA34; Tue, 9 Apr 2013 16:50:27 +0900 (JST) Date: Tue, 9 Apr 2013 16:50:27 +0900 From: Simon Horman To: Jesse Gross Cc: "dev@openvswitch.org" , netdev , Ravi K , Isaku Yamahata , Ben Pfaff Subject: Re: [PATCH 1/4] Add packet recirculation Message-ID: <20130409075024.GG11444@verge.net.au> References: <1365403431-18102-1-git-send-email-horms@verge.net.au> <1365403431-18102-2-git-send-email-horms@verge.net.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Organisation: Horms Solutions Ltd. User-Agent: Mutt/1.5.21 (2010-09-15) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, Apr 08, 2013 at 06:46:29PM -0700, Jesse Gross wrote: > On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman wrote: > > diff --git a/datapath/actions.c b/datapath/actions.c > > index e9634fe..7b0f022 100644 > > --- a/datapath/actions.c > > +++ b/datapath/actions.c > > @@ -617,6 +617,9 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, > > case OVS_ACTION_ATTR_SAMPLE: > > err = sample(dp, skb, a); > > break; > > + > > + case OVS_ACTION_ATTR_RECIRCULATE: > > + return 1; > > I think that if we've had a previous output action with the port > stored in prev_port then this will cause the packet to not actually be > output. I'm not so sure. I see something like this occurring: 1. Iteration of for loop for output action switch (nla_type(a)) { case OVS_ACTION_ATTR_OUTPUT: prev_port = nla_get_u32(a); break; ... } 2. Iteration of of for loop for next action, lets say its is recirculate i. Output packet if (prev_port != -1) { do_output(dp, skb_clone(skb, GFP_ATOMIC), prev_port); prev_port = -1; } ii. Return due to recirculate switch (nla_type(a)) { ... case OVS_ACTION_ATTR_RECIRCULATE: return 1; } Am I missing something? > > diff --git a/datapath/datapath.c b/datapath/datapath.c > > index e8be795..ab39dd7 100644 > > --- a/datapath/datapath.c > > +++ b/datapath/datapath.c > > void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb) > [...] > > + if (IS_ERR_OR_NULL(skb)) { > > + break; > > + } else if (unlikely(!limit--)) { > > Should this be a predecrement? I will make it so. > > + kfree_skb(skb); > > Should we log some kind of rate limited warning here? Sure. > > + return; > > In the first case we use break to exit the loop and here we use > return. Both should have the same effect so it might be nice to make > them the same. > > > @@ -901,6 +913,9 @@ static int validate_and_copy_actions__(const struct nlattr *attr, > > skip_copy = true; > > break; > > > > + case OVS_ACTION_ATTR_RECIRCULATE: > > + break; > > I think we might want to jump out the loop here to better model how > the actions are actually executed. Sure, perhaps something like this? > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index e4a2f75..31255f6 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > dp_netdev_port_input(struct dp_netdev *dp, struct dp_netdev_port *port, > > struct ofpbuf *packet) > [...] > > + } else { > > + dp->n_missed++; > > + dp_netdev_output_userspace(dp, packet, DPIF_UC_MISS, &key, NULL); > > + recirculate = false; > > + } > > + } while (recirculate && limit--); > > I have the same question about predecrement here. I will change this one too. > > @@ -1163,6 +1190,7 @@ dp_netdev_sample(struct dp_netdev *dp, > > const struct nlattr *subactions = NULL; > > const struct nlattr *a; > > size_t left; > > + uint32_t skb_mark; > > I don't think it's right to have a new (and uninitialized) copy of > skb_mark here. We should have the same one all the way through, like > we do in the kernel. Sure. I will pass it as an argument to dp_netdev_sample() > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 47830c1..5129da1 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > I'm still working on more detailed comments for this. However, I'm > concerned about whether the behavior for revalidation and stats is > correct. I am a little concerned about that too. Perhaps Ben could look over it? --- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/datapath/datapath.c b/datapath/datapath.c index ab39dd7..721a52c 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -914,7 +914,7 @@ static int validate_and_copy_actions__(const struct nlattr *attr, break; case OVS_ACTION_ATTR_RECIRCULATE: - break; + goto out; default: return -EINVAL; @@ -926,6 +926,7 @@ static int validate_and_copy_actions__(const struct nlattr *attr, } } +out: if (rem > 0) return -EINVAL;