Message ID | 20130409075024.GG11444@verge.net.au |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, Apr 9, 2013 at 12:50 AM, Simon Horman <horms@verge.net.au> wrote: > On Mon, Apr 08, 2013 at 06:46:29PM -0700, Jesse Gross wrote: >> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@verge.net.au> 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? Sorry, you're right. >> > @@ -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/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; Since this function is now both validating and copying I think this will result in the recirculate action not being copied. >> > 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 rephrase, there are problems in both of those areas. Validation in particular I don't think handles resubmitted facets and I believe that stats on rules will be the sum of all resubmitted passes. Both of these will likely significantly affect the data structures, so please look into this before we go further. In general, I'd also like to see patches that are standalone without needing follow on patches to fix known problems (for example, the recirculation ID patches or MPLS GSO) unless there is a good reason. -- 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
On Tue, Apr 09, 2013 at 08:44:02AM -0700, Jesse Gross wrote: > On Tue, Apr 9, 2013 at 12:50 AM, Simon Horman <horms@verge.net.au> wrote: > > On Mon, Apr 08, 2013 at 06:46:29PM -0700, Jesse Gross wrote: > >> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@verge.net.au> 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? > > Sorry, you're right. > > >> > @@ -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/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; > > Since this function is now both validating and copying I think this > will result in the recirculate action not being copied. Thanks, I'll look into that. > >> > 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 rephrase, there are problems in both of those areas. Validation in > particular I don't think handles resubmitted facets and I believe that > stats on rules will be the sum of all resubmitted passes. Some questions: By resubmitted do you mean recirculated? What is the stats behaviour that you would like? With regards to validation, I assume the area of concern is around facet_revalidate(). I will look into that. > Both of these will likely significantly affect the data structures, so > please look into this before we go further. Sure. I was not planning to push (much) further until this series is reviewed properly. > In general, I'd also like > to see patches that are standalone without needing follow on patches > to fix known problems (for example, the recirculation ID patches or > MPLS GSO) unless there is a good reason. Thanks, I understand. I'll try and structure my patches accordingly. -- 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
On Wed, Apr 10, 2013 at 2:16 AM, Simon Horman <horms@verge.net.au> wrote: > On Tue, Apr 09, 2013 at 08:44:02AM -0700, Jesse Gross wrote: >> On Tue, Apr 9, 2013 at 12:50 AM, Simon Horman <horms@verge.net.au> wrote: >> > On Mon, Apr 08, 2013 at 06:46:29PM -0700, Jesse Gross wrote: >> >> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@verge.net.au> wrote: >> >> > 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 rephrase, there are problems in both of those areas. Validation in >> particular I don't think handles resubmitted facets and I believe that >> stats on rules will be the sum of all resubmitted passes. > > Some questions: > By resubmitted do you mean recirculated? Yes. > What is the stats behaviour that you would like? A given rule should have byte and packet counts equal to the number of times it is matched (i.e. the first time) even if we have to decompose it into multiple passes internally. > With regards to validation, I assume the area of concern > is around facet_revalidate(). I will look into that. Yes. >> Both of these will likely significantly affect the data structures, so >> please look into this before we go further. > > Sure. I was not planning to push (much) further until this series > is reviewed properly. I'm planning on waiting on further reviews of this file until you've had a chance to look into validation and stats since I think that may change some of the data structures. -- 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
On Wed, Apr 10, 2013 at 09:21:23AM -0700, Jesse Gross wrote: > On Wed, Apr 10, 2013 at 2:16 AM, Simon Horman <horms@verge.net.au> wrote: > > On Tue, Apr 09, 2013 at 08:44:02AM -0700, Jesse Gross wrote: > >> On Tue, Apr 9, 2013 at 12:50 AM, Simon Horman <horms@verge.net.au> wrote: > >> > On Mon, Apr 08, 2013 at 06:46:29PM -0700, Jesse Gross wrote: > >> >> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@verge.net.au> wrote: > >> >> > 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 rephrase, there are problems in both of those areas. Validation in > >> particular I don't think handles resubmitted facets and I believe that > >> stats on rules will be the sum of all resubmitted passes. > > > > Some questions: > > By resubmitted do you mean recirculated? > > Yes. > > > What is the stats behaviour that you would like? > > A given rule should have byte and packet counts equal to the number of > times it is matched (i.e. the first time) even if we have to decompose > it into multiple passes internally. > > > With regards to validation, I assume the area of concern > > is around facet_revalidate(). I will look into that. > > Yes. > > >> Both of these will likely significantly affect the data structures, so > >> please look into this before we go further. > > > > Sure. I was not planning to push (much) further until this series > > is reviewed properly. > > I'm planning on waiting on further reviews of this file until you've > had a chance to look into validation and stats since I think that may > change some of the data structures. Sure, I assumed as much. I'll try and prepare and post a version with those issues, and the other ones you raised elsewhere in your review, in the not to distant future. -- 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;