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