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<[email protected]>
CC: David Tsai<[email protected]>
Signed-off-by: Ben Pfaff<[email protected]>
---
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 [email protected]
Teemu Koponen [email protected]
Vishal Swarankar [email protected]
Yongqiang Liu [email protected]
+Zoltan Kiss [email protected]
kk yap [email protected]
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);
