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
