On 6/10/14, Thomas Graf <tg...@suug.ch> wrote:
> On 06/10/14 at 07:03pm, Avinash wrote:
>> Kernel recognizes and accepts ethertype of provider VLANs (ETH_P_8021AD)
>> along with normal VLAN (ETH_P_8021Q). Also in flow key formation,
>> only the outermost VLAN is considered. Remaining stacked VLANs are
>> skipped.
>>
>> Signed-off-by: Avinash <avinashand...@gmail.com>
>
> Something seems to have corrupted this patch as tabs have been
> replaced with spaces.
>
Yes. I will be sending the correct patch along with the cover letter
as mentioned by you in the other mail.
>>
>> +static int parse_remaining_vlans(struct sk_buff *skb)
>> +{
>> + struct qtag_prefix {
>> + __be16 eth_type;
>> + __be16 tci;
>> + };
>> +
>> + struct qtag_prefix *qp;
>> + if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>> sizeof(__be16)))
>> + return 0;
>> +
>> + if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
>> + sizeof(__be16))))
>> + return -ENOMEM;
>> +
>> + qp = (struct qtag_prefix *) skb->data;
>> + while ((qp->eth_type == htons(ETH_P_8021Q) ||
>> + qp->eth_type == htons(ETH_P_8021AD)))
>
> Seeing all of these scattered around, I wonder whether it makes sense
> to have something like:
>
> static inline bool
> is_vlan_ether_type(const __be16 type)
> {
> return type == htons(ETH_P_8021Q) || type == htons(ETH_P_8021AD);
> }
>
>
Thanks. It makes more sense and also increases the readability of the code.
Will incorporate the change.
>> + {
>> + __skb_pull(skb, sizeof(struct qtag_prefix));
>> + qp = (struct qtag_prefix *) skb->data;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static __be16 parse_ethertype(struct sk_buff *skb)
>> {
>> struct llc_snap_hdr {
>> @@ -471,10 +497,14 @@ int ovs_flow_extract(struct sk_buff *skb, u16
>> in_port, struct sw_flow_key *key)
>>
>> if (vlan_tx_tag_present(skb))
>> key->eth.tci = htons(vlan_get_tci(skb));
>> - else if (eth->h_proto == htons(ETH_P_8021Q))
>> + else if (eth->h_proto == htons(ETH_P_8021Q) ||
>> + eth->h_proto == htons(ETH_P_8021AD))
>> if (unlikely(parse_vlan(skb, key)))
>> return -ENOMEM;
>>
>> + if (unlikely(parse_remaining_vlans(skb)))
>> + return -ENOMEM;
>
> How about adding the functionality of parse_remaining_vlans() to
> parse_vlan() directly instead of checking every single packet?
>
Combining parse_remaining_vlans() with the parse_vlan() might not skip
the inner VLANs present in the packet when the execution enters the
condition vlan_tx_tag_present().
For example, if an ARP packet having mulitpl VLANs (say double VLANs)
is received as shown below
ARP Request
| ETHER ADDR | 88A8 | 15 | 8100 | 10 | 806 |
During the formation of flowkey, the execution initially enters the condition
if (vlan_tx_tag_present)
key->eth.tci = htons(vlan_get_tci(skb)); // 15;
but the remaining VLANs are still present as part of skb->data.
| 8100 | 10 | 806 |
In this case, the function parse_ethertype() return 0x8100 instead of
actual ARP type as the remaining VLANs are not skipped.
--
AVINASH
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
