diff mbox

[1/4] Add packet recirculation

Message ID 20130409075024.GG11444@verge.net.au
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Simon Horman April 9, 2013, 7:50 a.m. UTC 
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?

> > 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

Comments

Jesse Gross April 9, 2013, 3:44 p.m. UTC | #1 
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
Simon Horman April 10, 2013, 9:16 a.m. UTC | #2 
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
Jesse Gross April 10, 2013, 4:21 p.m. UTC | #3 
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
Simon Horman April 11, 2013, 12:14 a.m. UTC | #4 
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 mbox

Patch

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;