Hi Zang,
Thanks for reporting this bug. As I see it, the check on dp_hash != 0 in
ofproto-dpif-xlate.c is there to guarantee that a dp_hash value has been
computed for the packet once before, not necessarily that a new one is computed
for each translated select group. That's why a check for a valid dp_hash is OK.
Bat all datapaths must adhere to the invariant that valid dp_hash !=0. Indeed
the kernel datapath implements this:
datapath/linux/actions.c:
1071 static void execute_hash(struct sk_buff *skb, struct sw_flow_key *key,
1072 const struct nlattr *attr)
1073 {
1074 struct ovs_action_hash *hash_act = nla_data(attr);
1075 u32 hash = 0;
1076
1077 /* OVS_HASH_ALG_L4 is the only possible hash algorithm. */
1078 hash = skb_get_hash(skb);
1079 hash = jhash_1word(hash, hash_act->hash_basis);
1080 if (!hash)
1081 hash = 0x1;
1082
1083 key->ovs_flow_hash = hash;
1084 }
The correct fix in my view would be to implement the same for the netdev
datapath in lib/odp-execute.c.
This requirement on the dp_hash action implementations should better be
documented properly.
BR, Jan
> -----Original Message-----
> From: ovs-dev-boun...@openvswitch.org <ovs-dev-boun...@openvswitch.org> On
> Behalf Of Zang MingJie
> Sent: Tuesday, 25 September, 2018 10:45
> To: ovs dev <d...@openvswitch.org>
> Subject: [ovs-dev] Bug: select group with dp_hash causing recursive
> recirculation
>
> Hi, we found a serious problem where one pmd is stop working, I want to
> share the problem and find solution here.
>
> vswitchd log:
>
> 2018-09-13T23:36:44.377Z|40269235|dpif_netdev(pmd45)|WARN|Packet dropped.
> Max recirculation depth exceeded.
> 2018-09-13T23:36:44.387Z|40269236|dpif_netdev(pmd45)|WARN|Packet dropped.
> Max recirculation depth exceeded.
> 2018-09-13T23:36:44.391Z|40269237|dpif_netdev(pmd45)|WARN|Packet dropped.
> Max recirculation depth exceeded.
>
> problematic datapath flows:
>
>
> ct_state(+new-est),recirc_id(0x143c893),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=443),
> packets:84573093, bytes:6308807903, used:0.009s,
> flags:SFPRU.ECN[200][400][800],
> actions:meter(306),hash(hash_l4(0)),recirc(0x237b09d)
>
>
> recirc_id(0x237b09d),in_port(2),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no),
> packets:279713339, bytes:20890205186, used:0.007s,
> flags:SFPRU.ECN[200][400][800], actions:hash(hash_l4(0)),recirc(0x237b09d)
>
> corresponding openflow:
>
> cookie=0x5b5ab65e000f0101, duration=4848269.642s, table=40,
> n_packets=974343805, n_bytes=72484367083,
> priority=10,tcp,metadata=0xf010000000000/0xffffff0000000000,tp_dst=443
> actions=group:983297
>
>
> group_id=983297,type=select,selection_method=dp_hash,bucket=bucket_id:3057033848,weight:100,actions=ct(commit,table=70,zo
> ne=15,exec(nat(dst=10.177.251.203:443))),...``lots
> of buckets``...
>
>
>
> Following explains how select group with dp_hash works.
>
> To implement select group with dp_hash, two datapath flows are needed:
>
> 1. calculate dp_hash, recirculate to second one
> 2. select group bucket by dp_hash
>
> When encounter a datapath miss, openflow doesn't know which one is missing,
> so it depends on dp_hash value of the packet:
>
> if dp_hash == 0 generate first dp flow.
> if dp_hash != 0 generate second dp flow.
>
>
> Back to the problem.
>
> Notice that second datapath flow is a dead loop, it recirculate to itself.
> The cause of the problem is here ofproto/ofproto-dpif-xlate.c#L4429[1]:
>
> /* dp_hash value 0 is special since it means that the dp_hash has not
> been
> * computed, as all computed dp_hash values are non-zero. Therefore
> * compare to zero can be used to decide if the dp_hash value is valid
> * without masking the dp_hash field. */
> if (!dp_hash) {
>
> The comment saying that `dp_hash` shouldn't be zero, but under DPDK, it can
> be zero, at lib/odp-execute.c#L747[2]
>
> /* RSS hash can be used here instead of 5tuple for
> * performance reasons. */
> if (dp_packet_rss_valid(packet)) {
> hash = dp_packet_get_rss_hash(packet);
> hash = hash_int(hash, hash_act->hash_basis);
> } else {
> flow_extract(packet, &flow);
> hash = flow_hash_5tuple(&flow, hash_act->hash_basis);
> }
> packet->md.dp_hash = hash;
>
> I don't know how small chance that `hash_int` returns 0, we have tested
> that if the final hash is 0, will definitely trigger the same bug. And due
> to the chance is extremely low, I'm also investigation that if there are
> other situation that will pass 0 hash to ofp.
>
>
>
> IMO, it is silly to depends on dp_hash value, maybe we need a new mechanism
> which can pass data between ofp and odp freely. And a quick solution could
> be just change the 0 hash to 1.
>
>
> [1]
> https://github.com/openvswitch/ovs/blob/master/ofproto/ofproto-dpif-xlate.c#L4429
> [2] https://github.com/openvswitch/ovs/blob/master/lib/odp-execute.c#L747
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
