On Nov 19, 2014, at 6:44 PM, Jesse Gross <je...@nicira.com> wrote:
> On Wed, Nov 19, 2014 at 3:33 PM, Jarno Rajahalme <jrajaha...@nicira.com>
> wrote:
>>
>>
>>
>> Jarno
>>
>>
>>> On Nov 19, 2014, at 2:21 PM, Jesse Gross <je...@nicira.com> wrote:
>>>
>>>> On Wed, Nov 19, 2014 at 12:51 PM, Jarno Rajahalme <jrajaha...@nicira.com>
>>>> wrote:
>>>>
>>>>> On Nov 17, 2014, at 2:33 PM, Jesse Gross <je...@nicira.com> wrote:
>>>>>
>>>>>> On Mon, Nov 17, 2014 at 1:50 PM, Lori Jakab <loja...@cisco.com> wrote:
>>>>>>> On 11/17/14 8:03 PM, Jesse Gross wrote:
>>>>>>>> On Mon, Nov 17, 2014 at 9:24 AM, Lorand Jakab <loja...@cisco.com>
>>>>>>>> wrote:
>>>>>>>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>>>>>>>> index 54510c8..8ca3469 100644
>>>>>>>> --- a/datapath/flow_netlink.c
>>>>>>>> +++ b/datapath/flow_netlink.c
>>>>>>>> @@ -692,6 +692,18 @@ static int metadata_from_nlattrs(struct
>>>>>>>> sw_flow_match *match, u64 *attrs,
>>>>>>>> else
>>>>>>>> SW_FLOW_KEY_PUT(match, phy.is_layer3, true,
>>>>>>>> is_mask);
>>>>>>>> }
>>>>>>>> + /* Layer 3 packets from user space have the EtherType as
>>>>>>>> metadata
>>>>>>>> */
>>>>>>>> + if (*attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
>>>>>>>
>>>>>>> Is this correct? I thought that EtherType wasn't serialized in this
>>>>>>> case - in other places we extract this from the IP/IPv6 attribute
>>>>>>> directly.
>>>>>>
>>>>>>
>>>>>> For "ovs_flow" Netlink messages we can do that, but not for "ovs_packet"
>>>>>> messages, which only have packet metadata, not the full flow key. Packet
>>>>>> metadata didn't include EtherType until now, but I unless we use the
>>>>>> nibble
>>>>>> from the IP version, we need to add it for layer 3 packets only.
>>>>>
>>>>> Hmm, I see. I think that diverging the Netlink encoding for flow
>>>>> installation vs. metadata is probably not a good idea over the long
>>>>> term. If I remember correctly, I believe the reason for only encoding
>>>>> part of the flow key for metadata was to reduce serialization cost.
>>>>> Jarno is thinking about combining the packet execution with flow
>>>>> installation, which would might make this a moot point. If that's the
>>>>> case, then we can probably solve this issue by just using the full
>>>>> flow key.
>>>>
>>>> I have a patch that adds an optional packet to the flow installation
>>>> command. However, we need to keep the existing packet execution command
>>>> for backwards compatibility. Also, there are cases (like exceeded flow
>>>> limit), where we will not add new flows, but just execute the miss
>>>> packets. This means that this problem needs to be solved independently of
>>>> packet execution.
>>>
>>> The source of the problem is that we send part of the flow key instead
>>> of the whole thing, which I believe was only done as an optimization.
>>> If this becomes a rare case rather than the common one, I think we can
>>> just remove the optimization and things will continue to work in all
>>> cases.
>>
>> Would adding just the ethertype to the serialized metadata for packet
>> execute be unthinkable? It's still a subset of the whole flow key.
>
> In the case of L3 packets, the full serialization actually doesn't
> include the EtherType so it would be deviating from a strict subset.
> My thinking on netlink serialization is that sticking as close as
> possible to what's actually in the packet is the best way to evolve
> the protocol without having weird corner cases in the future.
>
The netlink key for flow put is serialized from struct flow, so even in the
case of L3 the ethertype would be serialized.
Even the kernel datapath is encoding the ethertype in key_extract(), even when
it is not in the packet:
+ /* Link layer. */
+ if (key->phy.is_layer3) {
+ key->eth.tci = 0;
+ key->eth.type = skb->protocol;
+ } else {
So I guess you are not saying that the ethertype should not be serialized, due
to it not being in the L3 packet?
> Do you remember how much benefit this optimization actually provided
> in the first place?
Sorry, no i don’t.
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
