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.
>> <snip>
>>
>>> + stats_basic = stats_attrs[TCA_STATS_BASIC];
>>> + bs = nl_attr_get_unspec(stats_basic, sizeof *bs);
>>> +
>>> + stats->n_packets.lo = bs->packets;
>>> + stats->n_packets.hi = 0;
>>> + stats->n_bytes.hi = bs->bytes >> 32;
>>> + stats->n_bytes.lo = bs->bytes & 0x00000000FFFFFFFF;
>>
>>
>> Should this use put_32aligned_u64()?
>
>
> yes. wanted to do that as followup commit later to avoid too
> many changes. I can merge it now though.
OK.
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#define TCA_ACT_MIN_PRIO 1
>>
>>
>> Stray define - Shouldn't this be in linux/pkt_cls.h?
>>
>
> it's not in pkt_cls.h, we only have TCA_ACT_MAX_PRIO
> We can also see in act_api.c in kernel that they loop from 1
> and don't use a definition.
> we added a local definition for readability.
Did you consider suggesting a patch upstream to add it to pkt_cls.h,
which may assist readability there too?
>>> +
>>> + tcmsg = tc_make_request(ifindex, RTM_GETTFILTER, NLM_F_DUMP,
>>> &request);
>>> + tcmsg->tcm_parent = tc_make_handle(0xffff, 0);
>>
>>
>> Do TC people usually define handles in terms of these specific
>> integers? I wonder if a subsequent follow-up patch would improve
>> readability by #defining these tc_make_handle(0xffff, 0) numbers to
>> show what they actually mean - like ingress, root.
>>
>
> currently we copied it from existing functions that already set
> tcm_parent like this.
> In tc we can use the word 'ingress' which then in the code is
> translating to:
>
> req.t.tcm_parent = TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS);
>
> we can do a followup patch to do this as well or even on this add
> another macro.
Thanks, I think this would help.
>>> +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?
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
