netdev - Re: [net-next PATCH v3 3/8] net: sched: add cls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56C48295.9030806@gmail.com>
Date:	Wed, 17 Feb 2016 06:24:21 -0800
From:	John Fastabend <john.fastabend@...il.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>, jiri@...nulli.us,
	amir@...ai.me, davem@...emloft.net
CC:	netdev@...r.kernel.org, jeffrey.t.kirsher@...el.com
Subject: Re: [net-next PATCH v3 3/8] net: sched: add cls_u32 offload hooks
 for netdevs

On 16-02-17 02:59 AM, Jamal Hadi Salim wrote:
> On 16-02-17 12:17 AM, John Fastabend wrote:
>> This patch allows netdev drivers to consume cls_u32 offloads via
>> the ndo_setup_tc ndo op.
>>
>> This works aligns with how network drivers have been doing qdisc
>> offloads for mqprio.
>>
> 
> This one i have comments on.
> 
>> Signed-off-by: John Fastabend <john.r.fastabend@...el.com>
>> ---
>>   include/linux/netdevice.h |    6 ++-
>>   include/net/pkt_cls.h     |   34 +++++++++++++++
>>   net/sched/cls_u32.c       |   99
>> ++++++++++++++++++++++++++++++++++++++++++++-

[...]

>>   #endif /* CONFIG_NET_CLS_IND */
>>
>> +struct tc_cls_u32_knode {
>> +    struct tcf_exts *exts;
>> +    u8 fshift;
>> +    u32 handle;
>> +    u32 val;
>> +    u32 mask;
>> +    u32 link_handle;
>> +    struct tc_u32_sel *sel;
>> +};
>>
> 
> Swapping sel and fshift would give better struct alignment.
> 

Makes sense I went ahead and did this.

>> +struct tc_cls_u32_hnode {
>> +    u32 handle;
>> +    u32 prio;
>> +    unsigned int divisor;
>> +};
> 
> 
> Assuming in the future "prio" would be moved to something that is more
> generic classifier specific?
> 

Sure in the future it can be moved up into a generic struct if
it becomes useful there.

>> +enum tc_clsu32_command {
>> +    TC_CLSU32_NEW_KNODE,
>> +    TC_CLSU32_REPLACE_KNODE,
>> +    TC_CLSU32_DELETE_KNODE,
>> +    TC_CLSU32_NEW_HNODE,
>> +    TC_CLSU32_REPLACE_HNODE,
>> +    TC_CLSU32_DELETE_HNODE,
>> +};
>> +
> 
> It seems to me commands should be generic which speak
> Netlinkism. A REPLACE is just a flag to NEW. You dont need
> a NEW_XXX for every object. switchdev got this right.
> If you use cmd + flags  then you can have all kinds of
> netlink semantics that relay user intent from user space. Example:
> Exclusivity where user says "create if it doesnt exist but dont replace
> if it does".
> At minimal add "flags" there.
> Maybe not this release - but it makes sense to move "command" into
> tc_to_netdev; a u16 cmd + u16 flags.
> 

Yep next set of patches add the specific hw/sw/both semantics
and specific error handling strategies. For this series we just
get the simplest one.

>> +struct tc_cls_u32_offload {
>> +    /* knode values */
>> +    enum tc_clsu32_command command;
>> +    union {
>> +        struct tc_cls_u32_knode knode;
>> +        struct tc_cls_u32_hnode hnode;
>> +    };
>> +};
>> +
>>   #endif
>> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
>> index 4fbb674..d54bc94 100644
> 
>> +static void u32_replace_hw_hnode(struct tcf_proto *tp, struct
>> tc_u_hnode *h)
>> +{
>> +    struct net_device *dev = tp->q->dev_queue->dev;
>> +    struct tc_cls_u32_offload u32_offload = {0};
>> +    struct tc_to_netdev offload;
>> +
>> +    offload.type = TC_SETUP_CLSU32;
>> +    offload.cls_u32 = &u32_offload;
>> +
>> +    if (dev->netdev_ops->ndo_setup_tc) {
>> +        offload.cls_u32->command = TC_CLSU32_NEW_HNODE;
> 
> TC_CLSU32_REPLACE_HNODE?
> 

Yep I made this change and will send out v4.

[...]

> 
> 
> You are unconditionally calling the _hw_ api. For someone not using _hw_
> offloads, there are a few more instructions. Maybe just do the
> dev->netdev_ops->ndo_setup_tc first?
> 

My simple add/rem stress test didn't seem to make any difference.
I think there is so much other stuff going on here this is in the
noise. I'll take a pass at optimizing this later for all
cases not just the hw loading ones.

> 
> And to Or's point: How do i distinguish s/w from h/w?

Next series handles this for now just enable the simplest case.

> 
> cheers,
> jamal
>