[ovs-dev] [PATCH vlan-maint] ofproto: Preserve in_port across trips through the datapath.
Ben Pfaff
blp at nicira.com
Tue Jul 10 22:20:46 UTC 2012
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.kiss at citrix.com>
> >>>CC: David Tsai<dtsai at nicira.com>
> >>>Signed-off-by: Ben Pfaff<blp at 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-hama at cb.jp.nec.com
> >>> Teemu Koponen koponen at nicira.com
> >>> Vishal Swarankar vishal.swarnkar at gmail.com
> >>> Yongqiang Liu liuyq7809 at gmail.com
> >>>+Zoltan Kiss zoltan.kiss at citrix.com
> >>> kk yap yapkke at 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);
> >
>
More information about the dev
mailing list