OK, I've pushed my bug fix to vlan-maint.
I haven't actually reviewed your optimization for correctness. I'll be
out most of the day today, then I'll review it tomorrow.
On Wed, Jul 11, 2012 at 03:42:48PM +0100, Zoltan Kiss wrote:
> 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
