Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows
On Wed, 11 Apr 2018 13:55:11 -0700, Michael Chan wrote: > On Wed, Apr 11, 2018 at 1:50 PM, Andy Gospodarek wrote: > > On Wed, Apr 11, 2018 at 01:41:31PM -0700, Michael Chan wrote: > > True, but I'm not sure that tc_cls_common_offload is used in all cases. > > Take red_offload() as one of those. > > For Flower, we know we have the extack pointer in > tc_cls_common_offload struct and we can use it to set the netlink > error message. The point is that we don't have to modify > ndo_setup_tc(). Yes, the extack is actually only populated when skip_sw is specified to avoid warning users who don't care about offloads. Flower offloads don't go via .ndo_setup_tc but TC block callbacks. But one day we will hopefully find a reasonable way to pass extack to qdisc offloads as well.. FWIW your driver is actually already using extack under the veil of tc_cls_can_offload_and_chain0() :)
Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows
On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote: > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct > bnxt_tc_flow *flow) > return false; > } > > + /* Currently source/dest MAC cannot be partial wildcard */ > + if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) && > + !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) { > + netdev_info(bp->dev, "Wildcard match unsupported for Source > MAC\n"); This wouldn't be something to do in net, but how do you feel about using extack for messages like this? > + return false; > + }
Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows
On Wed, Apr 11, 2018 at 1:50 PM, Andy Gospodarek wrote: > On Wed, Apr 11, 2018 at 01:41:31PM -0700, Michael Chan wrote: >> On Wed, Apr 11, 2018 at 1:31 PM, Andy Gospodarek >> wrote: >> > On Wed, Apr 11, 2018 at 11:43:14AM -0700, Jakub Kicinski wrote: >> >> On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote: >> >> > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, >> >> > struct bnxt_tc_flow *flow) >> >> > return false; >> >> > } >> >> > >> >> > + /* Currently source/dest MAC cannot be partial wildcard */ >> >> > + if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) && >> >> > + !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) >> >> > { >> >> > + netdev_info(bp->dev, "Wildcard match unsupported for Source >> >> > MAC\n"); >> >> >> >> This wouldn't be something to do in net, but how do you feel about >> >> using extack for messages like this? >> >> >> > >> > I agree 'net' would not have been the place for a change like that, but >> > I do think that would be a good idea. It looks like we could easily >> > change the ndo_setup_tc to something like this: >> > >> > int (*ndo_setup_tc)(struct net_device *dev, >> > enum tc_setup_type type, >> > void *type_data, >> > struct netlink_ext_ack >> > *extack); >> >> I think the extack pointer is already in the tc_cls_common_offload >> struct inside tc_cls_flower_offload struct. > > True, but I'm not sure that tc_cls_common_offload is used in all cases. > Take red_offload() as one of those. For Flower, we know we have the extack pointer in tc_cls_common_offload struct and we can use it to set the netlink error message. The point is that we don't have to modify ndo_setup_tc().
Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows
On Wed, Apr 11, 2018 at 01:41:31PM -0700, Michael Chan wrote: > On Wed, Apr 11, 2018 at 1:31 PM, Andy Gospodarek > wrote: > > On Wed, Apr 11, 2018 at 11:43:14AM -0700, Jakub Kicinski wrote: > >> On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote: > >> > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, > >> > struct bnxt_tc_flow *flow) > >> > return false; > >> > } > >> > > >> > + /* Currently source/dest MAC cannot be partial wildcard */ > >> > + if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) && > >> > + !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) { > >> > + netdev_info(bp->dev, "Wildcard match unsupported for Source > >> > MAC\n"); > >> > >> This wouldn't be something to do in net, but how do you feel about > >> using extack for messages like this? > >> > > > > I agree 'net' would not have been the place for a change like that, but > > I do think that would be a good idea. It looks like we could easily > > change the ndo_setup_tc to something like this: > > > > int (*ndo_setup_tc)(struct net_device *dev, > > enum tc_setup_type type, > > void *type_data, > > struct netlink_ext_ack > > *extack); > > I think the extack pointer is already in the tc_cls_common_offload > struct inside tc_cls_flower_offload struct. True, but I'm not sure that tc_cls_common_offload is used in all cases. Take red_offload() as one of those.
Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows
On Wed, Apr 11, 2018 at 1:31 PM, Andy Gospodarek wrote: > On Wed, Apr 11, 2018 at 11:43:14AM -0700, Jakub Kicinski wrote: >> On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote: >> > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, >> > struct bnxt_tc_flow *flow) >> > return false; >> > } >> > >> > + /* Currently source/dest MAC cannot be partial wildcard */ >> > + if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) && >> > + !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) { >> > + netdev_info(bp->dev, "Wildcard match unsupported for Source >> > MAC\n"); >> >> This wouldn't be something to do in net, but how do you feel about >> using extack for messages like this? >> > > I agree 'net' would not have been the place for a change like that, but > I do think that would be a good idea. It looks like we could easily > change the ndo_setup_tc to something like this: > > int (*ndo_setup_tc)(struct net_device *dev, > enum tc_setup_type type, > void *type_data, > struct netlink_ext_ack > *extack); I think the extack pointer is already in the tc_cls_common_offload struct inside tc_cls_flower_offload struct.
Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows
On Wed, Apr 11, 2018 at 11:43:14AM -0700, Jakub Kicinski wrote: > On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote: > > @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, > > struct bnxt_tc_flow *flow) > > return false; > > } > > > > + /* Currently source/dest MAC cannot be partial wildcard */ > > + if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) && > > + !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) { > > + netdev_info(bp->dev, "Wildcard match unsupported for Source > > MAC\n"); > > This wouldn't be something to do in net, but how do you feel about > using extack for messages like this? > I agree 'net' would not have been the place for a change like that, but I do think that would be a good idea. It looks like we could easily change the ndo_setup_tc to something like this: int (*ndo_setup_tc)(struct net_device *dev, enum tc_setup_type type, void *type_data, struct netlink_ext_ack *extack); It also looks like most of the callers of ndo_setup_tc have infra in place to pass extack easily when the call is sourced from a netlink message. The others can just pass in NULL or define a local netlink_ext_ack variable for short-term use.
Re: [PATCH net v2 2/6] bnxt_en: do not allow wildcard matches for L2 flows
On Wed, Apr 11, 2018 at 11:43 AM, Jakub Kicinski wrote: > On Wed, 11 Apr 2018 11:50:14 -0400, Michael Chan wrote: >> @@ -764,6 +788,41 @@ static bool bnxt_tc_can_offload(struct bnxt *bp, struct >> bnxt_tc_flow *flow) >> return false; >> } >> >> + /* Currently source/dest MAC cannot be partial wildcard */ >> + if (bits_set(&flow->l2_key.smac, sizeof(flow->l2_key.smac)) && >> + !is_exactmatch(flow->l2_mask.smac, sizeof(flow->l2_mask.smac))) { >> + netdev_info(bp->dev, "Wildcard match unsupported for Source >> MAC\n"); > > This wouldn't be something to do in net, but how do you feel about > using extack for messages like this? > Sounds reasonable to me. Just need to pass in the extack pointer to this function to set the netlink error message.