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