On Mon, Nov 14, 2011 at 3:20 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Mon, Nov 14, 2011 at 02:37:37PM -0800, Jesse Gross wrote:
>> On Sat, Nov 12, 2011 at 1:01 PM, Ben Pfaff <b...@nicira.com> wrote:
>> > When the datapath was converted to use Netlink attributes for describing
>> > flow keys, I had a vague idea of how it could be smoothly extensible, but
>> > I didn't actually implement extensibility or carefully think it through.
>> > This commit adds a document that describes how flow keys can be extended
>> > in a compatible fashion and adapts the existing interface to match what
>> > it says.
>> >
>> > This commit doesn't actually implement extensibility. ??I already have a
>> > separate patch series out for that. ??This patch series borrows from that
>> > one heavily, but the extensibility series will need to be reworked
>> > somewhat once this one is in.
>> >
>> > This commit is only lightly tested because I don't have a good test setup
>> > for VLANs.
>> >
>> > Signed-off-by: Ben Pfaff <b...@nicira.com>
>>
>> I got some sparse errors with this:
>> /home/jesse/openvswitch/datapath/linux/flow.c:1091:29: warning: symbol
>> 'err' shadows an earlier one
>> /home/jesse/openvswitch/datapath/linux/flow.c:1010:13: originally declared
>> here
>> /home/jesse/openvswitch/datapath/linux/flow.c:1117:29: warning: symbol
>> 'err' shadows an earlier one
>> /home/jesse/openvswitch/datapath/linux/flow.c:1010:13: originally declared
>> here
>
> Odd, I still don't see those. Do you use any particular sparse flags?
Yes, I run it with the full flags:
make C=2 CF="-Wsparse-all -D__CHECK_ENDIAN__"
>> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> > index 966ef7a..3ac6673 100644
>> > --- a/include/linux/openvswitch.h
>> > +++ b/include/linux/openvswitch.h
>> > @@ -267,7 +267,7 @@ enum ovs_key_attr {
>> > ?? ?? ?? ??OVS_KEY_ATTR_PRIORITY, ??/* u32 skb->priority */
>> > ?? ?? ?? ??OVS_KEY_ATTR_IN_PORT, ?? /* u32 OVS dp port number */
>> > ?? ?? ?? ??OVS_KEY_ATTR_ETHERNET, ??/* struct ovs_key_ethernet */
>> > - ?? ?? ?? OVS_KEY_ATTR_8021Q, ?? ?? /* struct ovs_key_8021q */
>> > + ?? ?? ?? OVS_KEY_ATTR_VLAN, ?? ?? ??/* be16 VLAN TCI */
>> > ?? ?? ?? ??OVS_KEY_ATTR_ETHERTYPE, /* be16 Ethernet type */
>> > ?? ?? ?? ??OVS_KEY_ATTR_IPV4, ?? ?? ??/* struct ovs_key_ipv4 */
>> > ?? ?? ?? ??OVS_KEY_ATTR_IPV6, ?? ?? ??/* struct ovs_key_ipv6 */
>> > @@ -277,6 +277,7 @@ enum ovs_key_attr {
>> > ?? ?? ?? ??OVS_KEY_ATTR_ICMPV6, ?? ??/* struct ovs_key_icmpv6 */
>> > ?? ?? ?? ??OVS_KEY_ATTR_ARP, ?? ?? ?? /* struct ovs_key_arp */
>> > ?? ?? ?? ??OVS_KEY_ATTR_ND, ?? ?? ?? ??/* struct ovs_key_nd */
>> > + ?? ?? ?? OVS_KEY_ATTR_ENCAP, ?? ?? /* Nested set of encapsulated
>> > attributes. */
>>
>> Should we put this closer to the beginning of the list rather than
>> just mixed in the middle?
>
> I moved it to the top, right after UNSPEC. Do you want it somewhere
> else?
No, that makes sense.
>> > diff --git a/lib/odp-util.c b/lib/odp-util.c
>> > index c70ab11..0ca616b 100644
>> > --- a/lib/odp-util.c
>> > +++ b/lib/odp-util.c
>> > +parse_flow_nlattrs(const struct nlattr *key, size_t key_len,
>> > + ?? ?? ?? ?? ?? ?? ?? ?? ?? const struct nlattr *attrs[], uint64_t
>> > *present_attrsp)
>> > ??{
>> > ?? ?? static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>> > - ?? ??const struct nlattr *attrs[OVS_KEY_ATTR_MAX + 1];
>> > ?? ?? const struct nlattr *nla;
>> > - ?? ??uint64_t expected_attrs;
>> > ?? ?? uint64_t present_attrs;
>> > - ?? ??uint64_t missing_attrs;
>> > - ?? ??uint64_t extra_attrs;
>> > ?? ?? size_t left;
>> >
>> > - ?? ??memset(flow, 0, sizeof *flow);
>> > -
>> > - ?? ??memset(attrs, 0, sizeof attrs);
>> > + ?? ??memset(attrs, 0, (OVS_KEY_ATTR_MAX + 1) * sizeof *attrs);
>>
>> Is there a reason why userspace and kernel do duplicate checking
>> differently? The kernel does it based on present_attrs and userspace
>> does it based on the attribute stored in the array.
>
> I didn't want the overhead of memset'ing all 64*4 == 256 or 64*8 ==
> 512 bytes of the temporary array in the kernel, so I used the bitmap
> exclusively there to keep track of whether an attribute had been seen.
> But I'll change it to whichever way you prefer.
Yeah, the kernel version seemed a little nicer to me, so I was
actually wondering why we didn't do the same thing in userspace
(aren't both versions executed approximately the same number of times
and therefore the overhead has equal impact?).
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
