Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Thu, Apr 04, 2013 at 11:21:33AM +0900, Simon Horman wrote: On Wed, Apr 03, 2013 at 05:55:46PM -0700, Jesse Gross wrote: On Wed, Apr 3, 2013 at 5:24 PM, Simon Horman ho...@verge.net.au wrote: On Wed, Apr 03, 2013 at 01:29:53PM -0700, Jesse Gross wrote: On Wed, Apr 3, 2013 at 6:05 AM, Simon Horman ho...@verge.net.au wrote: On Wed, Apr 03, 2013 at 10:59:11AM +0900, Simon Horman wrote: On Tue, Apr 02, 2013 at 05:24:40PM -0700, Jesse Gross wrote: On Fri, Mar 22, 2013 at 6:44 AM, Simon Horman ho...@verge.net.au 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. Yes, I agree. I think I have something working there too. I think we could probably just add some kind of mark equivalent to the userspace datapath. It doesn't seem like it should be too difficult. 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. Long term, the right optimization is probably to handle all recirculation in userspace for these cases. It will certainly help performance in situations without facets since that means that we're already stressed on flow setups and it would be good to not generate more trips through the kernel. However, for the time being the best strategy seems like some kind of delayed allocation of facets to the time that we decide to do recirculation (unless we were going to create one anyways). Thanks, I'll think about this some more. Yes, I agree with your statement on performance. That is exactly what I was thinking of too. I am wondering if by handling recirculation in userspace you mean that the packet should be modified, or in other words executed, in userspace an then a fresh actions translation should occur? Right, we would end up doing all of the packet modifications and recirculations in userspace and the common case would be to just output in the kernel. If so, this seems similar to the code that I had in rfc1 of the recirculation patch. But it was rather complex in parts as I tried to handle recirculation of misses with facets in user-space too. And IIRC you asked me to remove it as you felt it duplicated datapath code. I think it should be possible to handle recirculation of just cases without facets in userspace in this way. But I'd like to confirm that is the direction you have in mind. I think this is potentially tricky to get right in all of the corner cases and your new version of the patch is certainly easier to read. It seems to me that it would be better to get something in that is correct (and doesn't affect performance for situations that don't use this, which is why I suggested on demand allocation of facets). We can then come back and optimize the performance after that. I agree that an incremental approach with smaller and more readable patches is desirable. And I believe that the behaviour in rfc2 and rfc3 is correct for handle flow miss without facets as it forces flow miss with facets if there is any chance of recirculation. What I am a little unclear about is how to handle packet out, which currently seems to be far away from anything relating to facets. I wonder if an approach would be to make an add-on patch or patches which adds support for ovs-vswitchd to handle recirculation. I believe much of such a change would involve re-using packet execution code that is already present for use by the user-space
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Wed, Apr 03, 2013 at 10:59:11AM +0900, Simon Horman wrote: On Tue, Apr 02, 2013 at 05:24:40PM -0700, Jesse Gross wrote: On Fri, Mar 22, 2013 at 6:44 AM, Simon Horman ho...@verge.net.au 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. Yes, I agree. I think I have something working there too. I think we could probably just add some kind of mark equivalent to the userspace datapath. It doesn't seem like it should be too difficult. 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. Long term, the right optimization is probably to handle all recirculation in userspace for these cases. It will certainly help performance in situations without facets since that means that we're already stressed on flow setups and it would be good to not generate more trips through the kernel. However, for the time being the best strategy seems like some kind of delayed allocation of facets to the time that we decide to do recirculation (unless we were going to create one anyways). Thanks, I'll think about this some more. Yes, I agree with your statement on performance. That is exactly what I was thinking of too. I am wondering if by handling recirculation in userspace you mean that the packet should be modified, or in other words executed, in userspace an then a fresh actions translation should occur? If so, this seems similar to the code that I had in rfc1 of the recirculation patch. But it was rather complex in parts as I tried to handle recirculation of misses with facets in user-space too. And IIRC you asked me to remove it as you felt it duplicated datapath code. I think it should be possible to handle recirculation of just cases without facets in userspace in this way. But I'd like to confirm that is the direction you have in mind. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Wed, Apr 3, 2013 at 6:05 AM, Simon Horman ho...@verge.net.au wrote: On Wed, Apr 03, 2013 at 10:59:11AM +0900, Simon Horman wrote: On Tue, Apr 02, 2013 at 05:24:40PM -0700, Jesse Gross wrote: On Fri, Mar 22, 2013 at 6:44 AM, Simon Horman ho...@verge.net.au 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. Yes, I agree. I think I have something working there too. I think we could probably just add some kind of mark equivalent to the userspace datapath. It doesn't seem like it should be too difficult. 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. Long term, the right optimization is probably to handle all recirculation in userspace for these cases. It will certainly help performance in situations without facets since that means that we're already stressed on flow setups and it would be good to not generate more trips through the kernel. However, for the time being the best strategy seems like some kind of delayed allocation of facets to the time that we decide to do recirculation (unless we were going to create one anyways). Thanks, I'll think about this some more. Yes, I agree with your statement on performance. That is exactly what I was thinking of too. I am wondering if by handling recirculation in userspace you mean that the packet should be modified, or in other words executed, in userspace an then a fresh actions translation should occur? Right, we would end up doing all of the packet modifications and recirculations in userspace and the common case would be to just output in the kernel. If so, this seems similar to the code that I had in rfc1 of the recirculation patch. But it was rather complex in parts as I tried to handle recirculation of misses with facets in user-space too. And IIRC you asked me to remove it as you felt it duplicated datapath code. I think it should be possible to handle recirculation of just cases without facets in userspace in this way. But I'd like to confirm that is the direction you have in mind. I think this is potentially tricky to get right in all of the corner cases and your new version of the patch is certainly easier to read. It seems to me that it would be better to get something in that is correct (and doesn't affect performance for situations that don't use this, which is why I suggested on demand allocation of facets). We can then come back and optimize the performance after that. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Wed, Apr 03, 2013 at 01:29:53PM -0700, Jesse Gross wrote: On Wed, Apr 3, 2013 at 6:05 AM, Simon Horman ho...@verge.net.au wrote: On Wed, Apr 03, 2013 at 10:59:11AM +0900, Simon Horman wrote: On Tue, Apr 02, 2013 at 05:24:40PM -0700, Jesse Gross wrote: On Fri, Mar 22, 2013 at 6:44 AM, Simon Horman ho...@verge.net.au 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. Yes, I agree. I think I have something working there too. I think we could probably just add some kind of mark equivalent to the userspace datapath. It doesn't seem like it should be too difficult. 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. Long term, the right optimization is probably to handle all recirculation in userspace for these cases. It will certainly help performance in situations without facets since that means that we're already stressed on flow setups and it would be good to not generate more trips through the kernel. However, for the time being the best strategy seems like some kind of delayed allocation of facets to the time that we decide to do recirculation (unless we were going to create one anyways). Thanks, I'll think about this some more. Yes, I agree with your statement on performance. That is exactly what I was thinking of too. I am wondering if by handling recirculation in userspace you mean that the packet should be modified, or in other words executed, in userspace an then a fresh actions translation should occur? Right, we would end up doing all of the packet modifications and recirculations in userspace and the common case would be to just output in the kernel. If so, this seems similar to the code that I had in rfc1 of the recirculation patch. But it was rather complex in parts as I tried to handle recirculation of misses with facets in user-space too. And IIRC you asked me to remove it as you felt it duplicated datapath code. I think it should be possible to handle recirculation of just cases without facets in userspace in this way. But I'd like to confirm that is the direction you have in mind. I think this is potentially tricky to get right in all of the corner cases and your new version of the patch is certainly easier to read. It seems to me that it would be better to get something in that is correct (and doesn't affect performance for situations that don't use this, which is why I suggested on demand allocation of facets). We can then come back and optimize the performance after that. I agree that an incremental approach with smaller and more readable patches is desirable. And I believe that the behaviour in rfc2 and rfc3 is correct for handle flow miss without facets as it forces flow miss with facets if there is any chance of recirculation. What I am a little unclear about is how to handle packet out, which currently seems to be far away from anything relating to facets. I wonder if an approach would be to make an add-on patch or patches which adds support for ovs-vswitchd to handle recirculation. I believe much of such a change would involve re-using packet execution code that is already present for use by the user-space datapath. From there I think it would be not to much of a leap to handle recirculation in ovs-vswitchd for flow misses without facets. And then we can see where we are. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Wed, Apr 03, 2013 at 05:55:46PM -0700, Jesse Gross wrote: On Wed, Apr 3, 2013 at 5:24 PM, Simon Horman ho...@verge.net.au wrote: On Wed, Apr 03, 2013 at 01:29:53PM -0700, Jesse Gross wrote: On Wed, Apr 3, 2013 at 6:05 AM, Simon Horman ho...@verge.net.au wrote: On Wed, Apr 03, 2013 at 10:59:11AM +0900, Simon Horman wrote: On Tue, Apr 02, 2013 at 05:24:40PM -0700, Jesse Gross wrote: On Fri, Mar 22, 2013 at 6:44 AM, Simon Horman ho...@verge.net.au 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. Yes, I agree. I think I have something working there too. I think we could probably just add some kind of mark equivalent to the userspace datapath. It doesn't seem like it should be too difficult. 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. Long term, the right optimization is probably to handle all recirculation in userspace for these cases. It will certainly help performance in situations without facets since that means that we're already stressed on flow setups and it would be good to not generate more trips through the kernel. However, for the time being the best strategy seems like some kind of delayed allocation of facets to the time that we decide to do recirculation (unless we were going to create one anyways). Thanks, I'll think about this some more. Yes, I agree with your statement on performance. That is exactly what I was thinking of too. I am wondering if by handling recirculation in userspace you mean that the packet should be modified, or in other words executed, in userspace an then a fresh actions translation should occur? Right, we would end up doing all of the packet modifications and recirculations in userspace and the common case would be to just output in the kernel. If so, this seems similar to the code that I had in rfc1 of the recirculation patch. But it was rather complex in parts as I tried to handle recirculation of misses with facets in user-space too. And IIRC you asked me to remove it as you felt it duplicated datapath code. I think it should be possible to handle recirculation of just cases without facets in userspace in this way. But I'd like to confirm that is the direction you have in mind. I think this is potentially tricky to get right in all of the corner cases and your new version of the patch is certainly easier to read. It seems to me that it would be better to get something in that is correct (and doesn't affect performance for situations that don't use this, which is why I suggested on demand allocation of facets). We can then come back and optimize the performance after that. I agree that an incremental approach with smaller and more readable patches is desirable. And I believe that the behaviour in rfc2 and rfc3 is correct for handle flow miss without facets as it forces flow miss with facets if there is any chance of recirculation. What I am a little unclear about is how to handle packet out, which currently seems to be far away from anything relating to facets. I wonder if an approach would be to make an add-on patch or patches which adds support for ovs-vswitchd to handle recirculation. I believe much of such a change would involve re-using packet execution code that is already present for use by the user-space datapath. From there I think it would be not to much of a leap to handle recirculation in ovs-vswitchd for flow misses without facets. And then we can see where we are. I
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Fri, Mar 22, 2013 at 6:44 AM, Simon Horman ho...@verge.net.au 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. I think we could probably just add some kind of mark equivalent to the userspace datapath. It doesn't seem like it should be too difficult. 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. Long term, the right optimization is probably to handle all recirculation in userspace for these cases. It will certainly help performance in situations without facets since that means that we're already stressed on flow setups and it would be good to not generate more trips through the kernel. However, for the time being the best strategy seems like some kind of delayed allocation of facets to the time that we decide to do recirculation (unless we were going to create one anyways). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Tue, Apr 02, 2013 at 05:24:40PM -0700, Jesse Gross wrote: On Fri, Mar 22, 2013 at 6:44 AM, Simon Horman ho...@verge.net.au 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. Yes, I agree. I think I have something working there too. I think we could probably just add some kind of mark equivalent to the userspace datapath. It doesn't seem like it should be too difficult. 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. Long term, the right optimization is probably to handle all recirculation in userspace for these cases. It will certainly help performance in situations without facets since that means that we're already stressed on flow setups and it would be good to not generate more trips through the kernel. However, for the time being the best strategy seems like some kind of delayed allocation of facets to the time that we decide to do recirculation (unless we were going to create one anyways). Thanks, I'll think about this some more. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
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
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
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
* 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).
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.
@@ -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.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Fri, Mar 15, 2013 at 7:27 AM, Simon Horman ho...@verge.net.au wrote:
Recirculation is a technique to allow a frame to re-enter
frame processing. This is intended to be used after actions
have been applied to the frame with modify the frame in
some way that makes it possible for richer processing to occur.
An example is and indeed targeted use case is MPLS. If an MPLS frame has an
mpls_pop action applied with the IPv4 ethernet type then it becomes
possible to decode the IPv4 portion of the frame. This may be used to
construct a facet that modifies the IPv4 portion of the frame. This is not
possible prior to the mpls_pop action as the contents of the frame after
the MPLS stack is not known to be IPv4.
Design:
* New recirculation action.
ovs-vswitchd adds a recirculation action to the end of a list of
datapath actions for a flow when the actions are truncated because
insufficient flow match information is available to add the next
OpenFlow action. The recirculation action includes an id which can be
used to scope a facet lookup of a recirculated packet.
e.g. pop_mpls(0x0800),dec_ttl becomes pop_mpls(0x800),resubmit(id=1)
ovs-vswtichd then restarts processing of the packet, with truncated actions.
When the actions do not contain a recirculate action processing stops
and the mangled packet is executed.
If the packet is being processed by ovs-vswitchd as a miss and the miss
is being handled with facets (the usual case) then facets for each of
recirculation steps are added to the datapath.
For a packet that is not recirculated this will be a single facet. For
a packet that is recirculated once this will be two facets. And so on,
with the number of facets increasing by one for each recirculation.
* New recirculation_id match attribute
Facets of recirculated packets have a recirculation_id attribute added to
the match key, the id is that of the recirculation action that
caused the packet to be recirculated. This is used to scope the lookup
of facets.
* Datapath behaviour
Then the datapath encounters a recirculate action it:
+ Recalculates the flow key based on the packet
which will typically have been modified by previous actions
+ Adds a recirculation_id attribute to the match key whose id
is that of the recirculate action
+ Performs a lookup using the new match key
+ Processes the packet if a facet matches the key or;
+ Makes an upcall if necessary
TODO:
* More sensible handling of recirculation IDs [ovs-vswtichd]
* More sensible lookup of facets based on recirculation IDs [ovs-vswtichd]
* Look more closely at facet revalidation [ovs-vswtichd]
Signed-off-by: Simon Horman ho...@verge.net.au
I'm having trouble applying this (even after applying the earlier
patches in the series). I'm not sure what changed but would you mind
sending an updated version?
This is a somewhat short review, since I'm just looking at the diff.
diff --git a/datapath/actions.c b/datapath/actions.c
index 60fa71a..b75f479 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -634,6 +636,12 @@ static int do_execute_actions(struct datapath *dp,
struct sk_buff *skb,
case OVS_ACTION_ATTR_SAMPLE:
err = sample(dp, skb, a, tun_key);
break;
+
+ case OVS_ACTION_ATTR_RECIRCULATE:
+ if (recirculation_id) {
+ *recirculation_id = nla_get_u32(a);
+ }
OVS_CB seems like the perfect place to store recirculation_id, that
way we don't have to pass pointers around everywhere.
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:
* 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.
* It seems likely that if you were doing more than two passes of
recirculation then you would still only want to use a single ID so
it's not really desirable to remark on every pass.
@@ -686,23 +695,28 @@ int ovs_execute_actions(struct datapath *dp, struct
sk_buff *skb)
if (unlikely(++loop-count MAX_LOOPS))
loop-looping = true;
if (unlikely(loop-looping)) {
- error = loop_suppress(dp, acts);
- kfree_skb(skb);
+ printk(%s: looping, __func__);
Isn't this logging already handled by loop_suppress()?
I think the memory management of the skb is somewhat inconsistent -
the caller retains ownership in the case of recirculation and some
errors but the callee takes it for normal output actions. What if the
callee takes it in all cases (as today) and gives it back as a return
value for
Re: [ovs-dev] [RFC PATCH 4/4] Add packet recirculation
On Mon, Mar 18, 2013 at 01:49:43PM -0700, Jesse Gross wrote:
On Fri, Mar 15, 2013 at 7:27 AM, Simon Horman ho...@verge.net.au wrote:
Recirculation is a technique to allow a frame to re-enter
frame processing. This is intended to be used after actions
have been applied to the frame with modify the frame in
some way that makes it possible for richer processing to occur.
An example is and indeed targeted use case is MPLS. If an MPLS frame has an
mpls_pop action applied with the IPv4 ethernet type then it becomes
possible to decode the IPv4 portion of the frame. This may be used to
construct a facet that modifies the IPv4 portion of the frame. This is not
possible prior to the mpls_pop action as the contents of the frame after
the MPLS stack is not known to be IPv4.
Design:
* New recirculation action.
ovs-vswitchd adds a recirculation action to the end of a list of
datapath actions for a flow when the actions are truncated because
insufficient flow match information is available to add the next
OpenFlow action. The recirculation action includes an id which can be
used to scope a facet lookup of a recirculated packet.
e.g. pop_mpls(0x0800),dec_ttl becomes pop_mpls(0x800),resubmit(id=1)
ovs-vswtichd then restarts processing of the packet, with truncated
actions.
When the actions do not contain a recirculate action processing stops
and the mangled packet is executed.
If the packet is being processed by ovs-vswitchd as a miss and the miss
is being handled with facets (the usual case) then facets for each of
recirculation steps are added to the datapath.
For a packet that is not recirculated this will be a single facet. For
a packet that is recirculated once this will be two facets. And so on,
with the number of facets increasing by one for each recirculation.
* New recirculation_id match attribute
Facets of recirculated packets have a recirculation_id attribute added to
the match key, the id is that of the recirculation action that
caused the packet to be recirculated. This is used to scope the lookup
of facets.
* Datapath behaviour
Then the datapath encounters a recirculate action it:
+ Recalculates the flow key based on the packet
which will typically have been modified by previous actions
+ Adds a recirculation_id attribute to the match key whose id
is that of the recirculate action
+ Performs a lookup using the new match key
+ Processes the packet if a facet matches the key or;
+ Makes an upcall if necessary
TODO:
* More sensible handling of recirculation IDs [ovs-vswtichd]
* More sensible lookup of facets based on recirculation IDs [ovs-vswtichd]
* Look more closely at facet revalidation [ovs-vswtichd]
Signed-off-by: Simon Horman ho...@verge.net.au
I'm having trouble applying this (even after applying the earlier
patches in the series). I'm not sure what changed but would you mind
sending an updated version?
Sure, will do.
In the mean time you can see the code applied here:
https://github.com/horms/openvswitch/tree/devel/mpls-recirculate.rfc1
This is a somewhat short review, since I'm just looking at the diff.
diff --git a/datapath/actions.c b/datapath/actions.c
index 60fa71a..b75f479 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -634,6 +636,12 @@ static int do_execute_actions(struct datapath *dp,
struct sk_buff *skb,
case OVS_ACTION_ATTR_SAMPLE:
err = sample(dp, skb, a, tun_key);
break;
+
+ case OVS_ACTION_ATTR_RECIRCULATE:
+ if (recirculation_id) {
+ *recirculation_id = nla_get_u32(a);
+ }
OVS_CB seems like the perfect place to store recirculation_id, that
way we don't have to pass pointers around everywhere.
Sure, that sounds reasonable.
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?
* 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?
* It seems likely that if you were doing more than two passes of
recirculation then you would still only want to use a single ID so
it's not really desirable to remark on every pass.
@@ -686,23 +695,28 @@ int ovs_execute_actions(struct datapath *dp, struct
sk_buff *skb)
if (unlikely(++loop-count MAX_LOOPS))
loop-looping = true;
if
