Re: [dpdk-dev] [PATCH v5] net/i40e: implement hash function in rte flow API

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

Hi Chenxu,

<snip>


> [snip]
> 
> > > --- a/drivers/net/i40e/i40e_flow.c
> > > +++ b/drivers/net/i40e/i40e_flow.c
> > > @@ -4424,10 +4424,10 @@ i40e_flow_parse_qinq_filter(struct
> > > rte_eth_dev *dev,
> > >   * function for RSS, or flowtype for queue region configuration.
> > >   * For example:
> > >   * pattern:
> > > - * Case 1: only ETH, indicate  flowtype for queue region will be parsed.
> > > - * Case 2: only VLAN, indicate user_priority for queue region will be
> parsed.
> > > - * Case 3: none, indicate RSS related will be parsed in action.
> > > - * Any pattern other the ETH or VLAN will be treated as invalid except
> END.
> > > + * Case 1: try to transform patterns to pctype. valid pctype will be
> > > + *         used in parse action.
> > > + * Case 2: only ETH, indicate flowtype for queue region will be parsed.
> > > + * Case 3: only VLAN, indicate user_priority for queue region will be
> parsed.
> > >   * So, pattern choice is depened on the purpose of configuration of
> > >   * that flow.
> > >   * action:
> > > @@ -4438,15 +4438,66 @@ static int
> > >  i40e_flow_parse_rss_pattern(__rte_unused struct rte_eth_dev *dev,
> > >       const struct rte_flow_item *pattern,
> > >       struct rte_flow_error *error,
> > > -     uint8_t *action_flag,
> > > +     struct i40e_rss_pattern_info *p_info,
> > >       struct i40e_queue_regions *info)  {  const struct
> > > rte_flow_item_vlan *vlan_spec, *vlan_mask;  const struct
> > > rte_flow_item *item = pattern;  enum rte_flow_item_type item_type;
> > > -
> > > -if (item->type == RTE_FLOW_ITEM_TYPE_END)
> > > +struct rte_flow_item *items;
> > > +uint32_t item_num = 0; /* non-void item number of pattern*/
> > > +uint32_t i = 0; static const struct { enum rte_flow_item_type
> > > +*item_array; uint64_t type; } i40e_rss_pctype_patterns[] = { {
> > > +pattern_fdir_ipv4,
> > > +ETH_RSS_FRAG_IPV4 |
> > > ETH_RSS_NONFRAG_IPV4_OTHER },
> > > +{ pattern_fdir_ipv4_tcp, ETH_RSS_NONFRAG_IPV4_TCP }, {
> > > +pattern_fdir_ipv4_udp, ETH_RSS_NONFRAG_IPV4_UDP }, {
> > > +pattern_fdir_ipv4_sctp, ETH_RSS_NONFRAG_IPV4_SCTP }, {
> > > +pattern_fdir_ipv6,
> > > +ETH_RSS_FRAG_IPV6 |
> > > ETH_RSS_NONFRAG_IPV6_OTHER },
> > > +{ pattern_fdir_ipv6_tcp, ETH_RSS_NONFRAG_IPV6_TCP }, {
> > > +pattern_fdir_ipv6_udp, ETH_RSS_NONFRAG_IPV6_UDP }, {
> > > +pattern_fdir_ipv6_sctp, ETH_RSS_NONFRAG_IPV6_SCTP }, };
> > > +
> > > +p_info->types = I40E_RSS_TYPE_INVALID;
> > > +
> > > +if (item->type == RTE_FLOW_ITEM_TYPE_END) { p_info->types =
> > > +I40E_RSS_TYPE_NONE;
> > >  return 0;
> > > +}
> > > +
> > > +/* convert flow to pctype  */
> > > +while ((pattern + i)->type != RTE_FLOW_ITEM_TYPE_END) { if
> > > +((pattern
> > > ++ i)->type != RTE_FLOW_ITEM_TYPE_VOID) item_num++;
> > > +i++;
> > > +}
> > > +item_num++;
> > > +
> > > +items = rte_zmalloc("i40e_pattern",
> > > +    item_num * sizeof(struct rte_flow_item), 0); if (!items) {
> > > +rte_flow_error_set(error, ENOMEM,
> > > RTE_FLOW_ERROR_TYPE_ITEM_NUM,
> > > +   NULL, "No memory for PMD internal
> > > items.");
> > > +return -ENOMEM;
> > > +}
> > > +
> > > +i40e_pattern_skip_void_item(items, pattern);
> > > +
> > > +for (i = 0; i < RTE_DIM(i40e_rss_pctype_patterns); i++) { if
> > > (i40e_match_pattern(i40e_rss_pctype_patterns[i].item_array,
> > > +items)) {
> > > +p_info->types = i40e_rss_pctype_patterns[i].type; rte_free(items);
> > > +return 0; } }
> > > +
> > > +rte_free(items);
> > >
> > >  for (; item->type != RTE_FLOW_ITEM_TYPE_END; item++) {  if
> > > (item->last) { @@ -4459,7 +4510,7 @@
> > > i40e_flow_parse_rss_pattern(__rte_unused struct rte_eth_dev *dev,
> > > item_type = item->type;  switch (item_type) {  case
> > > RTE_FLOW_ITEM_TYPE_ETH:
> > > -*action_flag = 1;
> > > +p_info->action_flag = 1;
> > >  break;
> > >  case RTE_FLOW_ITEM_TYPE_VLAN:
> > >  vlan_spec = item->spec;
> > > @@ -4472,7 +4523,7 @@ i40e_flow_parse_rss_pattern(__rte_unused
> > > struct rte_eth_dev *dev,
> > >  vlan_spec->tci) >> 13) & 0x7;
> > >  info->region[0].user_priority_num = 1;  info->queue_region_number =
> > > 1; -*action_flag = 0;
> > > +p_info->action_flag = 0;
> > >  }
> > >  }
> > >  break;
> > > @@ -4500,12 +4551,14 @@ i40e_flow_parse_rss_pattern(__rte_unused
> > > struct rte_eth_dev *dev,
> > >   * max index should be 7, and so on. And also, queue index should be
> > >   * continuous sequence and queue region index should be part of rss
> > >   * queue index for this port.
> > > + * For hash params, the pctype in action and pattern must be same.
> > > + * Set queue index or symmetric hash enable must be with non-types.
> > >   */
> > >  static int
> > >  i40e_flow_parse_rss_action(struct rte_eth_dev *dev,
> > >      const struct rte_flow_action *actions,
> > >      struct rte_flow_error *error,
> > > -    uint8_t action_flag,
> > > +struct i40e_rss_pattern_info p_info,
> > >      struct i40e_queue_regions *conf_info,
> > >      union i40e_filter_t *filter)
> > >  {
> > > @@ -4516,7 +4569,7 @@ i40e_flow_parse_rss_action(struct rte_eth_dev
> > > *dev,  struct i40e_rte_flow_rss_conf *rss_config =
> > > &filter->rss_conf; struct i40e_rte_flow_rss_conf *rss_info =
> > > &pf->rss_info; -uint16_t i, j, n, tmp;
> > > +uint16_t i, j, n, tmp, nb_types;
> > >  uint32_t index = 0;
> > >  uint64_t hf_bit = 1;
> > >
> > > @@ -4535,7 +4588,7 @@ i40e_flow_parse_rss_action(struct rte_eth_dev
> > > *dev,  return -rte_errno;  }
> > >
> > > -if (action_flag) {
> > > +if (p_info.action_flag) {
> > >  for (n = 0; n < 64; n++) {
> > >  if (rss->types & (hf_bit << n)) {
> > >  conf_info->region[0].hw_flowtype[0] = n; @@ -4674,11 +4727,11 @@
> > > i40e_flow_parse_rss_action(struct rte_eth_dev *dev,  if
> > > (rss_config->queue_region_conf)  return 0;
> > >
> > > -if (!rss || !rss->queue_num) {
> > > +if (!rss) {
> > >  rte_flow_error_set(error, EINVAL,
> > >  RTE_FLOW_ERROR_TYPE_ACTION,
> > >  act,
> > > -"no valid queues");
> > > +"no valid rules");
> > >  return -rte_errno;
> > >  }
> > >
> > > @@ -4692,19 +4745,40 @@ i40e_flow_parse_rss_action(struct
> > > rte_eth_dev *dev,  }  }
> > >
> > > -if (rss_info->conf.queue_num) {
> > > -rte_flow_error_set(error, EINVAL,
> > > -RTE_FLOW_ERROR_TYPE_ACTION,
> > > -act,
> > > -"rss only allow one valid rule");
> > > -return -rte_errno;
> > > +if (rss->queue_num && (p_info.types || rss->types))
> >
> > Should the line above be
> >
> > if (conf_info->queue_region_number && (p_info.types || rss->types))
> >
> > to allow RSS configuration of types and queues in a single rule, for example:
> >
> > flow create 0 ingress pattern eth / end actions rss types udp end
> > queues  2 3 end / end
> >
> 
> For the conf_info->queue_region_number  and rss->queue_num, In the old
> codes, if there is eth or vlan, the conf_info->queue_region_number  will be
> set as 1 in the function parse pattern.
> And in the parse action function it will check the conf_info-
> >queue_region_number.
> If conf_info->queue_region_number == 1, it will do some things and return.
> It will not do the things but do others while conf_info-
> >queue_region_number == 0.
> And after parse it will call the function i40e_flush_queue_region_all_conf() if
> conf_info->queue_region_number == 1 While call the function
> i40e_config_rss_filter() if conf_info->queue_region_number == 0.
> 
> So what I changed is only when conf_info->queue_region_number == 0.
> 
> Btw, in i40e, the queue configuration is for a port, it can't be for one rule or
> one type.
> So I don't think it is a good idea for allowing RSS configuration of types and
> queues in a single rule.

Would you suggest two rules as follows?

To configure the queues:

flow create 0 ingress pattern end actions rss  queues  2 3 end / end

To configure the hash:

flow create 0 ingress pattern eth / ipv4 / end actions rss types ipv4 end key_len 0 queues end / end
(above rule is used on the ice pmd)

Regards,

Bernard