Re: [net-next PATCH v3 3/8] net: sched: add cls_u32 offload hooks for netdevs

[John Fastabend <john.fastabend@gmail.com>]

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@intel.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
>