On 1/2/2019 9:27 PM, Ben Pfaff wrote:
> On Wed, Jan 02, 2019 at 05:52:20PM +0000, Eli Britstein wrote:
>> On 1/2/2019 6:02 PM, Ben Pfaff wrote:
>>> On Wed, Jan 02, 2019 at 03:23:14PM +0000, Eli Britstein wrote:
>>>> Hello,
>>>>
>>>>
>>>>
>>>> Simple bridge, with 2 ports:
>>>>
>>>>
>>>>
>>>> ovs-vsctl show
>>>>
>>>> 66abf37d-391e-45ba-b740-d2d0f414cd15
>>>>
>>>> Bridge "br1"
>>>>
>>>> Port "enp4s0f0_1"
>>>>
>>>> Interface "enp4s0f0_1"
>>>>
>>>> Port "enp4s0f0_0"
>>>>
>>>> Interface "enp4s0f0_0"
>>>>
>>>> Port "br1"
>>>>
>>>> Interface "br1"
>>>>
>>>> type: internal
>>>>
>>>> ovs_version: "2.10.90"
>>>>
>>>>
>>>>
>>>> master version, last commit:
>>>>
>>>> 46df7fac7 netdev-tc-offloads: Support IPv6 hlimit rewrite
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> Single OF rule. Match on a SRCIP and rewrite to the same value:
>>>>
>>>> ovs-ofctl del-flows br1
>>>> ovs-ofctl add-flow br1
>>>> in_port=1,dl_dst=e4:11:22:33:44:51,dl_type=0x0800,nw_src=4.4.4.10,nw_dst=4.4.4.11,actions=mod_dl_dst=e4:11:22:33:aa:bb,mod_nw_src=4.4.4.10,mod_nw_dst=4.6.4.51,output:2
>>>>
>>>> ovs-ofctl dump-flows br1
>>>> cookie=0x0, duration=0.010s, table=0, n_packets=0, n_bytes=0,
>>>> ip,in_port="enp4s0f0_0",dl_dst=e4:11:22:33:44:51,nw_src=4.4.4.10,nw_dst=4.4.4.11
>>>>
>>>> actions=mod_dl_dst:e4:11:22:33:aa:bb,mod_nw_src:4.4.4.10,mod_nw_dst:4.6.4.51,output:"enp4s0f0_1"
>>>>
>>>> ovs-dpctl dump-flows --names
>>>> in_port(enp4s0f0_0),eth(dst=e4:11:22:33:44:51),eth_type(0x0800),ipv4(src=4.4.4.10,dst=4.4.4.11,proto=1,frag=no),
>>>> packets:3, bytes:294, used:3.510s,
>>>> actions:set(eth(dst=e4:11:22:33:aa:bb)),set(ipv4(src=4.4.4.10,dst=4.6.4.51)),enp4s0f0_1
>>>>
>>>> Rewrite to the same value of a matched field is not required. Can it be
>>>> filtered?
>>> The "set" actions are required because the Ethernet destination and IPv4
>>> destination actually change.
>> That is correct that there are changed fields. However, the src ip match
>> (nw_src=4.4.4.10) and the rewrite request (mod_nw_src:4.4.4.10) have the
>> same value. It's pointless to do a rewrite to the same value.
> OVS has masked set logic for these fields in commit_set_ipv4_action() in
> odp-util.c. If you track down why it's not active for you, please send
> a patch.
I tracked down the problem. There is only one "mask", so once we ask for
a rewrite for at least one field (in my example IPV4 dst), the rewrites
are applied for all the IPV4 fields that were matched.
Function do_xlate_actions in ofproto/ofproto-dpif-xlate.c works on
struct flow_wildcards *wc = ctx->wc which is the match mask. When
getting to case OFPACT_SET_IPV4_DST, it sets on the mask bits (but it's
already set from the match), but it changes the value of flow->nw_dst.
case OFPACT_SET_IPV4_DST:
if (flow->dl_type == htons(ETH_TYPE_IP)) {
memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
flow->nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4;
}
In lib/odp-util.c the entry point is commit_odp_actions, which as said
gets only one mask.
The commit_set... functions all call the static function "commit". In
this function we only compare the values of the whole set (IPV4 in my
example):
if (memcmp(key, base, size)) {
As requested one field from it, it is different. Then, to set the
rewrite actions we use the mask (from the match) so we rewrite all the
matched fields of IPV4.
I think there should be a match_mask and an action_mask. Then, we should
apply the rewrite by the action_mask.
However, I am not familiar with those code parts, and I see there are
many places to call do_xlate_actions in ofproto/ofproto-dpif-xlate.c,
with code paths I don't know.
Could I please ask for help from the community with a fix or with a
guidance how to fix it?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
