On Fri, Mar 22, 2013 at 10:44:23PM +0900, Simon Horman wrote:
> On Tue, Mar 19, 2013 at 09:01:27AM -0700, Jesse Gross wrote:
> > On Mon, Mar 18, 2013 at 6:34 PM, Simon Horman <ho...@verge.net.au> wrote:
> > > On Mon, Mar 18, 2013 at 01:49:43PM -0700, Jesse Gross wrote:
> > >> However, do we actually want to tie this directly to recirculation as
> > >> opposed to as a separate action? I see possible benefits of
> > >> separating it out:
> > >
> > > I'm not really sure that I understand. Could you explain
> > > how you see this working?
> >
> > Just a set action plus recirculation with no argument. Separating out
> > the issues of reusing skb mark, this could be done today with
> > something like:
> > pass 1: match MPLS -> pop_mpls, set(mark(X)), recirculate
> > pass 2: match mark X, match MPLS -> pop_mpls, recirculate
> > pass 3: match mark X, match IP -> output
>
> Thanks, I have a crude implementation of this working locally.
>
> I'm not sure what the implications are for the user-space datapath.
> Though that is less of a priority for me than the kernel datapath.
>
> > >> * It makes it more generic - we could potentially use skb_mark
> > >> directly or worst case introduce a new metadata field if we are
> > >> worried about conflicting uses (although we could always set the real
> > >> skb mark on the last pass). Either way there is a better potential
> > >> for reuse.
> > >
> > > I'm a bit wary of using skb_mark, what if its desirable to
> > > use it for something else?
> >
> > Strictly speaking, I don't think that it can conflict because
> > recirculation is completely internal to OVS where as all other uses of
> > mark are external. We can always restore the mark to whatever we want
> > before outputting (even if outputs and recirculation are interleaved,
> > we should have worst case we can set the appropriate value right
> > before each action).
>
> . Actually I found a bug which I will send a patch for shortly
> which seems to indicate that skb_mark matches are broken and thus
> not used (recently).
>
> > This might complicate a few cases but I think those are likely to be
> > very rare and it reduces our match size, which is the common pain
> > point.
> >
> > >> > + }
> > >> > + OVS_CB(skb)->flow = NULL;
> > >>
> > >> I think the check for having OVS_CB(skb)->flow already set is dead
> > >> code at this point and we don't need to special case it.
> > >
> > > Ok, I'll prepare a preparatory patch to remove the dead code
> > > and build on top of that. It should simplify the code a bit :)
> >
> > I actually fixed this after I noticed it here, so the dead code should
> > already be removed from master.
>
> Thanks, it does simply things a bit.
>
> > >> > @@ -905,10 +929,16 @@ static int ovs_packet_cmd_execute(struct sk_buff
> > >> > *skb, struct genl_info *info)
> > >> > goto err_unlock;
> > >> >
> > >> > local_bh_disable();
> > >> > - err = ovs_execute_actions(dp, packet);
> > >> > + err = ovs_execute_actions(dp, packet, NULL);
> > >> > local_bh_enable();
> > >> > rcu_read_unlock();
> > >> >
> > >> > + if (err > 0) {
> > >> > + /* Recirculation is invalid on packet execute */
> > >> > + err = -EINVAL;
> > >> > + goto err_flow_free;
> > >> > + }
> > >>
> > >> Isn't this going to add a lot of complication to userspace? It's
> > >> clearly possible for userspace to not use recirculation since it has
> > >> the full packet but it's basically going to require a separate
> > >> processing path just to handle this.
> > >
> > > I think that the path needs to be present in order to handle
> > > cases where facets aren't present. Such misses handled without facets
> > > and packet out handling.
> >
> > I think it's possible to handle those cases in the kernel but I can
> > see how that would add complications of its own. It's certainly easy
> > enough to disallow this for now and enable it in the future if
> > necessary.
>
> I am wondering if you could offer some advice on execution of
> a packet with a recirculate action. In particular, how to
> handle the in_port in the datapath as as far as I can see it is absent from
> an execute command.
Sorry, I was blind. I see that the in_port is available as metadata.
> I am also concerned, though less so, about:
>
> * How to handle packet_out. Perhaps some kind of synthetic facet?
> * How to detect if recirculation will occur and thus force
> a miss to be handled with a facet. For now I am just checking
> if the packet MPLS or not, but it would be nice not to force facets
> unless necessary. Actually, it would be nice not to force the
> use of facets at all.
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
