Hmm, I didn't remember ever sending you that round of feedback but indeed I see that all of my comments have been addressed.
Thanks, I'll take another look soon. On Tue, May 22, 2012 at 08:12:09PM +0200, ravi.ke...@telekom.com wrote: > Thanks Ben. I have addressed all your previous comments with one > exception of adding ofpbuf_* common function. I haven't worked on > the patch since mid April, will work on it after I receive > additional comments on the latest patch. > > Please note that latest patch has gone through additional testing > > vlan + mpls (ICMP) > vlan-qinq + mpls (ICMP) > > (both kernel and user-space) > > vlan + mpls (TCP offload) > mpls (tcp offload) > vlan-qinq (tcp offload) > > TCP offload is not perfect yet but works fine so far. I am looking into > performance numbers which doesn't look quite right. > > Thanks, > Ravi > > -----Original Message----- > From: Ben Pfaff [mailto:b...@nicira.com] > Sent: Tuesday, May 22, 2012 10:49 AM > To: Kerur, Ravi > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] MPLS and VLAN QinQ patch > > On Tue, May 22, 2012 at 07:36:32PM +0200, ravi.ke...@telekom.com wrote: > > Attached MPLS and VLAN QinQ patch after rebasing to following commit > > > > commit 046f1f89e6d7716581de207dd0c54421926bc25b > > Author: Ethan Jackson <et...@nicira.com<mailto:et...@nicira.com>> > > Date: Mon May 21 13:20:18 2012 -0700 > > > > Patch(s) have undergone additional integration testing and incorporates > > initial code review comments from Ben. > > Hi Ravi. Here's some feedback that I had started writing up on a > previous version of the patch that you sent me in private email some > time ago. Will you please apply any parts of it that still apply to > the current version of the MPLS patch, then post a revised version? > > Thanks, > > Ben. > > It applies, compiles, and passes "sparse" cleanly. Great! > > I don't see any handling for decrementing a TTL of 0. That's supposed > to be sent to the controller with OFPR_INVALID_TTL. > > include/linux/openvswitch.h > --------------------------- > > I think these should say be16 and be32 since their operands are > in network byte order: > + OVS_ACTION_ATTR_PUSH_MPLS, /* u16, ethertype */ > + OVS_ACTION_ATTR_POP_MPLS, /* u16, ethertype */ > + OVS_ACTION_ATTR_SET_MPLS_LSE, /* u32, mpls label stack entry */ > > > include/openflow/nicira-ext.h > ----------------------------- > > You can drop this line, it doesn't fit in in the context: > + /* MPLS related definitions*/ > > lib/dpif-netdev.c > ----------------- > > I don't think there's a benefit to the temporary vars here: > + case OVS_ACTION_ATTR_PUSH_MPLS: > + push_ethertype = nl_attr_get_be16(a); > + push_mpls(packet, push_ethertype); > + break; > + > + case OVS_ACTION_ATTR_POP_MPLS: > + pop_ethertype = nl_attr_get_be16(a); > + pop_mpls(packet, pop_ethertype); > + break; > + > + case OVS_ACTION_ATTR_SET_MPLS_LSE: > + mpls_lse = nl_attr_get_be32(a); > + set_mpls_lse(packet, mpls_lse); > + break; > > lib/flow.c > ---------- > > In flow_extract(), I don't think the cast is necessary: > + packet->l2_5 = (uint32_t *)b.data; > > lib/odp-util.c > -------------- > > I think that these should be ovs_be<N> types: > + case OVS_ACTION_ATTR_PUSH_MPLS: return sizeof(uint16_t); > + case OVS_ACTION_ATTR_POP_MPLS: return sizeof(uint16_t); > + case OVS_ACTION_ATTR_SET_MPLS_LSE: return sizeof(uint32_t); > > format_odp_action() and parse_odp_action() use different syntax for > push_mpls and pop_mpls (one includes "tpid=", the other doesn't). > Please make them consistent. > > (Is 'tpid' correct terminology for MPLS? I thought it was > VLAN-specific.) > > In parse_odp_action(), the numbers passed to strncmp() and returned > differ, but they should be the same. > > Code like this appears in a few places. Should we create a helper > function? > + htonl((mpls_label << MPLS_LABEL_SHIFT) | > + (mpls_tc << MPLS_TC_SHIFT) | > + (mpls_ttl << MPLS_TTL_SHIFT) | > + (mpls_stack << MPLS_STACK_SHIFT))); > The set_mpls_lse_values() function could also be implemented in terms > of such a helper. > > I'd like to see commit_mpls_action() split into a function to pop an > MPLS header and a function to push an MPLS header. The current > organization is confusing and I'm not convinced that it's 100% > correct. > > lib/packets.c > ------------- > > In set_mpls_lse_ttl(), please drop the unneeded parentheses here: > + *tag |= (ttl & htonl(MPLS_TTL_MASK)); > also in the other similar functions. > > I think the logic in dec_mpls_ttl() only works because the TTL is the > low 8 bits of mpls_lse. Conceptually there's a missing shift. > > copy_mpls_ttl_in() and copy_mpls_ttl_out() don't appear to check that > packet data is actually present. > > Also in those functions, missing {} here: > + if (mh == NULL || nh == NULL) > + return; > > The "return;" at the end of push_mpls_lse() may be omitted. > > push_mpls() and push_mpls_lse() look like they don't check that IPv4 > packet data is actually present. > > I think it's time to add ofpbuf_*() functions to insert and remove > data in the middle of a packet. It's hard to see whether open-coded > moves (there are a few in this file in addition to the ones you added > for MPLS) are correct, so it'd be nice to get it right once and reuse > it. > > ofproto/ofproto-dpif.c > ---------------------- > > facet_account() has some stray VLOG_DBG calls. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
