On 15 August 2017 at 01:19, Paul Blakey <[email protected]> wrote: > > > On 15/08/2017 10:28, Paul Blakey wrote: >> >> >> >> On 15/08/2017 04:04, Joe Stringer wrote: >>> >>> On 8 August 2017 at 07:21, Roi Dayan <[email protected]> wrote: >>>> >>>> From: Paul Blakey <[email protected]> >>>> >>>> Implement support for offloading ovs action set using >>>> tc header rewrite action. >>>> >>>> Signed-off-by: Paul Blakey <[email protected]> >>>> Reviewed-by: Roi Dayan <[email protected]> >>>> --- >>> >>> >>> <snip> >>> >>>> @@ -274,6 +346,48 @@ netdev_tc_flow_dump_destroy(struct netdev_flow_dump >>>> *dump) >>>> return 0; >>>> } >>>> >>>> +static void >>>> +parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf, >>>> + struct tc_flower *flower) >>>> +{ >>>> + char *mask = (char *) &flower->rewrite.mask; >>>> + char *data = (char *) &flower->rewrite.key; >>>> + >>>> + for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) { >>>> + char *put = NULL; >>>> + size_t nested = 0; >>>> + int len = ovs_flow_key_attr_lens[type].len; >>>> + >>>> + if (len <= 0) { >>>> + continue; >>>> + } >>>> + >>>> + for (int j = 0; j < ARRAY_SIZE(set_flower_map[type]); j++) { >>>> + struct netlink_field *f = &set_flower_map[type][j]; >>>> + >>>> + if (!f->size) { >>>> + break; >>>> + } >>> >>> >>> Seems like maybe there's similar feedback here to the previous patch >>> wrt sparsely populated array set_flower_map[]. >>> >> >> I'll answer there. > > > Scratch that, I actually meant to write it here. > > With this map we have fast (direct lookup) access on the put path > (parse_put_flow_set_masked_action) , and slow access on the reverse dump > path (parse_flower_rewrite_to_netlink_action) which iterates over all > indices, although it should skips them fast if they are empty. > > Changing this to sparse map will slow both of the paths, but dumping would > be faster. We didn't write this maps for performance but for convince of > adding new supported attributes.
OK, thanks for the explanation. I figured there must be a reason it was indexed that way, I just didn't see it in my first review pass through the series. Obviously fast path should be fast, and typically we don't worry quite as much about dumping fast (although I have in the past tested this to determine how many flows we will attempt to populate in adverse conditions). > What we can do to both maps is: > > 1) Using ovs thread once, we can create a fast (direct lookup) access map > for both paths - generating a reverse map at init time and using that > instead for dumping. Until there's a clear, known advantage I'm not sure if we should do this. > 2) Rewrite it in a non sparse way, use it for both. I think that we generally are more interested in 'put' performance so if this sacrifices it for 'dump' performance, then it's probably not a good idea (though it may still be too early to really optimize these). > 3) Rewrite it using a switch case, the logic for now is pretty much the same > for all attributes. > > What do you think? Do you think that this third option would improve the readability, allow more compile-time checking on the code, or more code reuse? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
