[ovs-dev] [PATCH vlan-maint] ofproto: Preserve in_port across trips through the datapath.
Zoltan Kiss
zoltan.kiss at citrix.com
Wed Jul 11 14:42:48 UTC 2012
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.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