On 5 June 2017 at 23:01, Roi Dayan <[email protected]> wrote: > > > On 05/06/2017 20:36, Joe Stringer wrote: >> >> On 3 June 2017 at 22:22, Roi Dayan <[email protected]> wrote: >>> >>> >>> >>> On 01/06/2017 20:53, Joe Stringer wrote: >>>> >>>> >>>> On 1 June 2017 at 07:39, Roi Dayan <[email protected]> wrote: >>>>> >>>>> >>>>> >>>>> >>>>> On 31/05/2017 03:50, Joe Stringer wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 28 May 2017 at 04:59, Roi Dayan <[email protected]> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Add tc helper functions to query and manipulate the flower >>>>>>> classifier. >>>>>>> >>>>>>> Signed-off-by: Paul Blakey <[email protected]> >>>>>>> Signed-off-by: Roi Dayan <[email protected]> >>>>>>> --- >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> 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(). >> > > maybe. that should also apply to tc_get_minor() to return > ovs_be16 instead of uint. > in parsing back from netlink to tc_flower we do: > flower->key.eth_type = (OVS_FORCE ovs_be16) tc_get_minor(tc->tcm_info);
That sounds reasonable to me, although I think that the function may be used for not just the tcm_info but also tcm_parent. > but a lot of callers use it for vlog and use %u format. > so changing it will probably also cause the compiler to complain. I'm guessing that not all callers assume it's supposed to denote an ethertype. > I'll skip this for V10 as I don't think this is critical > and we can do a commit later to update to what will be agreed on > and update all callers if necessary. That's fine. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
