| Message ID | OS3P286MB229551D6705894E6578778DCF5DB9@OS3P286MB2295.JPNP286.PROD.OUTLOOK.COM |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,net-next,v1,1/1] net: openvswitch: remove unnecessary vlan init in key_extract | expand |
On Tue, Feb 07, 2023 at 12:31:33PM +0800, Eddy Tao wrote: > Redefine clear_vlan to initialize one struct vlan_head > Define clear_vlans to initialize key.eth.vlan and key.eth.cvlan > Calls the revised functions accurately > > Reasoning: > > For vlan packet, current code calls clear_vlan unnecessarily, > since parse_vlan sets key->eth.vlan and key->eth.cvlan correctly. > Only special case where return value <=0 needs inialization > certail key.eth.vlan or key.eth.cvlan specifically. > > For none-vlan case, parse_vlan returns on the first parse_vlan_tag > which returns 0, in this case, calls clear_vlan > > For MAC_PROTO_NONE, logic is intact after this revision > > Signed-off-by: Eddy Tao <taoyuan_eddy@hotmail.com> This seems like a complex, and perhaps error-prone, way to avoid writing a few bytes. I do tend to think the extra code complexity is not worth it without some performance justification. OTOH, I think that perhaps a better question might be: do the bytes need to be cleared under any circumstances? I suspect key is discarded when an error occurs. > --- > net/openvswitch/flow.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c > index e20d1a973417..30a90597cab6 100644 > --- a/net/openvswitch/flow.c > +++ b/net/openvswitch/flow.c > @@ -480,12 +480,16 @@ static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *key_vh, > return 1; > } > > -static void clear_vlan(struct sw_flow_key *key) > +static inline void clear_vlan(struct vlan_head *vlan) > { > - key->eth.vlan.tci = 0; > - key->eth.vlan.tpid = 0; > - key->eth.cvlan.tci = 0; > - key->eth.cvlan.tpid = 0; > + vlan->tci = 0; > + vlan->tpid = 0; > +} > + > +static inline void clear_vlans(struct sw_flow_key *key) > +{ > + clear_vlan(&key->eth.vlan); > + clear_vlan(&key->eth.cvlan); > } This is a nice cleanup, IMHO :) > > static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) > @@ -498,14 +502,18 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) > } else { > /* Parse outer vlan tag in the non-accelerated case. */ > res = parse_vlan_tag(skb, &key->eth.vlan, true); > - if (res <= 0) > + if (res <= 0) { > + clear_vlans(key); I think this makes more sense in the caller. > return res; > + } > } > > /* Parse inner vlan tag. */ > res = parse_vlan_tag(skb, &key->eth.cvlan, false); > - if (res <= 0) > + if (res <= 0) { > + clear_vlan(&key->eth.cvlan); > return res; > + } > > return 0; > } > @@ -918,8 +926,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) > skb_reset_mac_header(skb); > > /* Link layer. */ > - clear_vlan(key); > if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) { > + clear_vlans(key); > if (unlikely(eth_type_vlan(skb->protocol))) > return -EINVAL; I think you missed the following case further down: if (unlikely(key->eth.type == htons(0))) return -ENOMEM;
Hi, Simon:
Thanks for looking into this. by looking into the compiled
instructions, i am convinced the patch is logically correct but not help
in performance, and it adds complexity,
Below function is actually optimized to one movq instruction with no
special -O settings on my centos8, which seems good enough for me
static void clear_vlan(struct sw_flow_key *key)
{
key->eth.vlan.tci = 0;
key->eth.vlan.tpid = 0;
key->eth.cvlan.tci = 0;
key->eth.cvlan.tpid = 0;
}
optimized to movq $0x0,0x158(%rbx)
I am withdrawing this patch
About your comment below, since it is after the parse_vlan handling, the
code looks fine and does not affect the logic of the patch
>>I think you missed the following case further down: >>>>if
(unlikely(key->eth.type == htons(0))) Have a great day
eddy
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c index e20d1a973417..30a90597cab6 100644 --- a/net/openvswitch/flow.c +++ b/net/openvswitch/flow.c @@ -480,12 +480,16 @@ static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *key_vh, return 1; } -static void clear_vlan(struct sw_flow_key *key) +static inline void clear_vlan(struct vlan_head *vlan) { - key->eth.vlan.tci = 0; - key->eth.vlan.tpid = 0; - key->eth.cvlan.tci = 0; - key->eth.cvlan.tpid = 0; + vlan->tci = 0; + vlan->tpid = 0; +} + +static inline void clear_vlans(struct sw_flow_key *key) +{ + clear_vlan(&key->eth.vlan); + clear_vlan(&key->eth.cvlan); } static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) @@ -498,14 +502,18 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) } else { /* Parse outer vlan tag in the non-accelerated case. */ res = parse_vlan_tag(skb, &key->eth.vlan, true); - if (res <= 0) + if (res <= 0) { + clear_vlans(key); return res; + } } /* Parse inner vlan tag. */ res = parse_vlan_tag(skb, &key->eth.cvlan, false); - if (res <= 0) + if (res <= 0) { + clear_vlan(&key->eth.cvlan); return res; + } return 0; } @@ -918,8 +926,8 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key) skb_reset_mac_header(skb); /* Link layer. */ - clear_vlan(key); if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) { + clear_vlans(key); if (unlikely(eth_type_vlan(skb->protocol))) return -EINVAL;
Redefine clear_vlan to initialize one struct vlan_head Define clear_vlans to initialize key.eth.vlan and key.eth.cvlan Calls the revised functions accurately Reasoning: For vlan packet, current code calls clear_vlan unnecessarily, since parse_vlan sets key->eth.vlan and key->eth.cvlan correctly. Only special case where return value <=0 needs inialization certail key.eth.vlan or key.eth.cvlan specifically. For none-vlan case, parse_vlan returns on the first parse_vlan_tag which returns 0, in this case, calls clear_vlan For MAC_PROTO_NONE, logic is intact after this revision Signed-off-by: Eddy Tao <taoyuan_eddy@hotmail.com> --- net/openvswitch/flow.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)