On 3 June 2017 at 22:22, Roi Dayan <r...@mellanox.com> wrote: > > > On 01/06/2017 20:53, Joe Stringer wrote: >> >> On 1 June 2017 at 07:39, Roi Dayan <r...@mellanox.com> wrote: >>> >>> >>> >>> On 31/05/2017 03:50, Joe Stringer wrote: >>>> >>>> >>>> On 28 May 2017 at 04:59, Roi Dayan <r...@mellanox.com> wrote: >>>>> >>>>> >>>>> Add tc helper functions to query and manipulate the flower classifier. >>>>> >>>>> Signed-off-by: Paul Blakey <pa...@mellanox.com> >>>>> Signed-off-by: Roi Dayan <r...@mellanox.com> >>>>> --- >>>> >>>> >>>> >>>> Again is this co-authored? utilities/checkpatch.py checks for stuff like >>>> this. >>>> >>>> I didn't go through all of the enums and make sure they're all >>>> covered, correct types, etc. >>>> >>>> <snip> >>>> >>>>> + [TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK] = { .type = NL_A_U32, .optional >>>>> = >>>>> true, }, >>>>> + [TCA_FLOWER_KEY_ENC_IPV4_DST_MASK] = { .type = NL_A_U32, .optional >>>>> = >>>>> true, }, >>>> >>>> >>>> >>>> Line lengths. >>>> >>> >>> ok >>> >>>> <snip> >>>> >>>>> +static void >>>>> +nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) >>>>> +{ >>>>> + unsigned long long int lastuse = tm->lastuse * 10; >>>> >>>> >>>> >>>> Where does the 10 come from? What does it mean? Should we have a #define >>>> for it? >>>> >>> >>> we want to convert here jiffies to ms and used some default value. >>> as Flavio suggested maybe do it in a macro? >>> later I saw in netdev-linux.c the functions read_psched() and >>> tc_ticks_to_bytes(). can do a followup commit to refactor those >>> somewhere else and use them in tc.c as well. >> >> >> Please do. > > > just to be clear here, is it ok to use a macro for this series > and do the followup commit after this series?
That sounds reasonable. >>>>> +int >>>>> +tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle, >>>>> + struct tc_flower *flower) >>>>> +{ >>>>> + struct ofpbuf request; >>>>> + struct tcmsg *tcmsg; >>>>> + struct ofpbuf *reply; >>>>> + int error = 0; >>>>> + size_t basic_offset; >>>>> + uint16_t eth_type = (OVS_FORCE uint16_t) flower->key.eth_type; >>>> >>>> >>>> >>>> Why does this need forcing? >>>> >>> >>> as eth_type is ove_be16 but tc_make_handle wants unsigned int. >>> we get a compilation warning from sparse about incorrect type. >>> this is to avoid sparse from failing. >> >> >> I see. Should it be unsigned int, then? Rather than casting a >> big-endian value to store a host-endian variable of the same size? >> > > it's not unsigned int to follow up with the rest of the user space code > that use ovs_be16 for eth_type. > also we are using match_set_dl_type() that expect ovs_be16. The rest of the code stores a big-endian eth_type in an ovs_be16, or host byte-order version in uint16_t. The thing I found surprising in this code was that the big endian version of the ethertype was being placed into a uint16_t without converting to host byte order. In the end, I think what we're trying to achieve is that the second parameter to tc_make_handle() should be a big-endian ethertype but sparse would complain if we passed it directly (because tc_make_handle()'s second argument type isn't ovs_be16). So OVS_FORCE is necessary, though I would have expected it to be used inside the arguments of tc_make_handle(). _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev