Yes, I would definitely want you to post your fix to vlan-maint, I've
also tested it.
Regarding my patch about optimization, we have reviewed it, and I think
it's not a crucial optimization for us (such PACKET_OUTs which generate
further PACKET_INs are supposed to be a rare case), so I don't insist to
include it to vlan-maint.
On 10/07/12 23:20, Ben Pfaff wrote:
Zoltan, let's consider each of these patches separately, since they
are independent.
Do you want me to the bug fix that I posted to the vlan-maint branch?
I've tested it and Justin has also reviewed it now.
Then, after that, we can talk about optimizations.
On Tue, Jul 10, 2012 at 09:06:39PM +0100, Zoltan Kiss wrote:
Indeed, my patch works only in a corner case, however it also
improves it a little bit. And this old tree is used in the latest
XenServer release 6.0.2, which will be here around for a couple of
years. So if you don't have any objection, I think it worth to keep
it.
On 10/07/12 18:36, Ben Pfaff wrote:
The patch I posted fixes the problem in the general case.
The patch you posted makes a difference in only one corner case, where
there is exactly one action in the kernel actions. The problem
remains in every other case. So, this patch is primarily an
optimization. It could possibly be applied on top of the patch that I
posted, although I don't know why you would want to optimize such an
old tree.
On Tue, Jul 10, 2012 at 06:28:54PM +0100, Zoltan Kiss wrote:
Hi,
I've came to the same conclusion about the cause of the bug, however
my proposed solution would use execute_odp_actions() to avoid the
round-trip to the kernel. See it in the attachment. Or is there any
specific reason to send it down to the kernel and receive it back
immediately?
I've tested this patch, and it solves this particular case.
Regards,
Zoltan Kiss
On 10/07/12 17:50, Ben Pfaff wrote:
When a "packet out" sent a packet to the datapath and then the datapath
sent the packet back via ODP_ACTION_ATTR_CONTROLLER, the in_port included
in the "packet out" was lost (it became OFPP_LOCAL) because the datapath's
"execute" operation doesn't accept an in_port.
This commit fixes the problem by including the in_port in the userdata
argument to ODP_ACTION_ATTR_CONTROLLER.
NICS-15.
Reported-by: Zoltan Kiss<zoltan.k...@citrix.com>
CC: David Tsai<dt...@nicira.com>
Signed-off-by: Ben Pfaff<b...@nicira.com>
---
Apologies if you receive multiple copies of this patch. This
version adds a CC to David Tsai.
AUTHORS | 1 +
ofproto/ofproto.c | 6 ++++--
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index 84cb387..9033908 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -80,6 +80,7 @@ Takayuki HAMA t-h...@cb.jp.nec.com
Teemu Koponen kopo...@nicira.com
Vishal Swarankar vishal.swarn...@gmail.com
Yongqiang Liu liuyq7...@gmail.com
+Zoltan Kiss zoltan.k...@citrix.com
kk yap yap...@stanford.edu
Thanks to all Open vSwitch contributors. If you are not listed above
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 37e2ad9..975e434 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2752,7 +2752,8 @@ xlate_output_action__(struct action_xlate_ctx *ctx,
&ctx->nf_output_iface, ctx->odp_actions);
break;
case OFPP_CONTROLLER:
- nl_msg_put_u64(ctx->odp_actions, ODP_ACTION_ATTR_CONTROLLER, max_len);
+ nl_msg_put_u64(ctx->odp_actions, ODP_ACTION_ATTR_CONTROLLER,
+ max_len | (ctx->flow.in_port<< 16));
break;
case OFPP_LOCAL:
add_output_action(ctx, ODPP_LOCAL);
@@ -4463,6 +4464,7 @@ handle_upcall(struct ofproto *p, struct dpif_upcall
*upcall)
case DPIF_UC_ACTION:
COVERAGE_INC(ofproto_ctlr_action);
odp_flow_key_to_flow(upcall->key, upcall->key_len,&flow);
+ flow.in_port = upcall->userdata>> 16;
send_packet_in(p, upcall,&flow, false);
break;
@@ -4902,7 +4904,7 @@ schedule_packet_in(struct ofconn *ofconn, struct
dpif_upcall *upcall,
send_len = MIN(send_len, ofconn->miss_send_len);
}
if (upcall->type == DPIF_UC_ACTION) {
- send_len = MIN(send_len, upcall->userdata);
+ send_len = MIN(send_len, upcall->userdata& 0xffff);
}
/* Copy or steal buffer for OFPT_PACKET_IN. */
# HG changeset patch
# Parent fda36ec74c2d4342d3d19e52c0f71a7e5e75a6a2
diff -r fda36ec74c2d ofproto/ofproto.c
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3152,7 +3152,7 @@ handle_packet_out(struct ofconn *ofconn,
{
struct ofproto *p = ofconn->ofproto;
struct ofp_packet_out *opo;
- struct ofpbuf payload, *buffer;
+ struct ofpbuf *buffer;
union ofp_action *ofp_actions;
struct action_xlate_ctx ctx;
struct ofpbuf *odp_actions;
@@ -3187,14 +3187,13 @@ handle_packet_out(struct ofconn *ofconn,
if (error || !buffer) {
return error;
}
- payload = *buffer;
} else {
- payload = request;
- buffer = NULL;
+ buffer = xmalloc(sizeof *buffer);
+ *buffer = request;
}
/* Extract flow, check actions. */
- flow_extract(&payload, 0, ofp_port_to_odp_port(ntohs(opo->in_port)),
+ flow_extract(buffer, 0, ofp_port_to_odp_port(ntohs(opo->in_port)),
&flow);
error = validate_actions(ofp_actions, n_ofp_actions,&flow, p->max_ports);
if (error) {
@@ -3202,10 +3201,11 @@ handle_packet_out(struct ofconn *ofconn,
}
/* Send. */
- action_xlate_ctx_init(&ctx, p,&flow,&payload);
+ action_xlate_ctx_init(&ctx, p,&flow, buffer);
odp_actions = xlate_actions(&ctx, ofp_actions, n_ofp_actions);
- dpif_execute(p->dpif, odp_actions->data, odp_actions->size,&payload);
+ execute_odp_actions(p,&flow, (const struct nlattr *)odp_actions->data,
odp_actions->size, buffer);
ofpbuf_delete(odp_actions);
+ return 0;
exit:
ofpbuf_delete(buffer);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
