Hi Simei, > -----Original Message----- > From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Andrew Rybchenko > Sent: Tuesday, October 1, 2019 3:49 PM > To: Su, Simei <simei...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>; Ye, > Xiaolong <xiaolong...@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v8 2/3] ethdev: extend RSS offload types > > On 10/1/19 5:36 PM, Simei Su wrote: > > This patch reserves several bits as input set selection from the high > > end of the 64 bits. It is combined with exisiting ETH_RSS_* to > > represent RSS types. > > > > Signed-off-by: Simei Su <simei...@intel.com> > > Reviewed-by: Qi Zhang <qi.z.zh...@intel.com> > > Acked-by: Ori Kam <or...@mellanox.com> > > --- > > lib/librte_ethdev/rte_ethdev.c | 22 ++++++++++++++++++++++ > > lib/librte_ethdev/rte_ethdev.h | 14 ++++++++++++++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/lib/librte_ethdev/rte_ethdev.c > > b/lib/librte_ethdev/rte_ethdev.c index af82360..5e5a974 100644 > > --- a/lib/librte_ethdev/rte_ethdev.c > > +++ b/lib/librte_ethdev/rte_ethdev.c > > @@ -1269,6 +1269,17 @@ struct rte_eth_dev * > > goto rollback; > > } > > > > + /* simplified the SRC/DST_ONLY RSS offload modificaiton */ > > + if (dev_conf->rx_adv_conf.rss_conf.rss_hf & > ETH_RSS_L3_SRC_ONLY && > > + dev_conf->rx_adv_conf.rss_conf.rss_hf & > ETH_RSS_L3_DST_ONLY) > > I'm afraid some compiler versions could bark about missing parenthesis here. > Consider to use something like: > (rss_conf->rss_hf & (ETH_RSS_L3_SRC_ONLY | ETH_RSS_L3_DST_ONLY) == > (ETH_RSS_L3_SRC_ONLY | ETH_RSS_L3_DST_ONLY)) May be parenthesis > around & is not required. > > > + dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf > &= > > + ~(ETH_RSS_L3_SRC_ONLY | > ETH_RSS_L3_DST_ONLY); > > + > > + if (dev_conf->rx_adv_conf.rss_conf.rss_hf & > ETH_RSS_L4_SRC_ONLY && > > + dev_conf->rx_adv_conf.rss_conf.rss_hf & > ETH_RSS_L4_DST_ONLY) > > + dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf > &= > > + ~(ETH_RSS_L4_SRC_ONLY | > ETH_RSS_L4_DST_ONLY); > > + > > I think it is wrong to duplicate the logic twice (here and below). > Helper static function should be used to avoid it. > > > /* Check that device supports requested rss hash functions. */ > > if ((dev_info.flow_type_rss_offloads | > > dev_conf->rx_adv_conf.rss_conf.rss_hf) != @@ -3112,6 +3123,17 > > @@ struct rte_eth_dev * > > if (ret != 0) > > return ret; > > > > + /* simplified the SRC/DST_ONLY RSS offload modificaiton */ > > + if (rss_conf->rss_hf & ETH_RSS_L3_SRC_ONLY && > > + rss_conf->rss_hf & ETH_RSS_L3_DST_ONLY) > > + rss_conf->rss_hf &= > > + ~(ETH_RSS_L3_SRC_ONLY | > ETH_RSS_L3_DST_ONLY); > > + > > + if (rss_conf->rss_hf & ETH_RSS_L4_SRC_ONLY && > > + rss_conf->rss_hf & ETH_RSS_L4_DST_ONLY) > > + rss_conf->rss_hf &= > > + ~(ETH_RSS_L4_SRC_ONLY | > ETH_RSS_L4_DST_ONLY); > > + > > dev = &rte_eth_devices[port_id]; > > if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) != > > dev_info.flow_type_rss_offloads) { diff --git > > a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h > > index 7722f70..6d61b84 100644 > > --- a/lib/librte_ethdev/rte_ethdev.h > > +++ b/lib/librte_ethdev/rte_ethdev.h > > @@ -505,6 +505,20 @@ struct rte_eth_rss_conf { > > #define ETH_RSS_GENEVE (1ULL << 20) > > #define ETH_RSS_NVGRE (1ULL << 21) > > > > +/* > > + * We use the following macros to combine with above ETH_RSS_* for > > + * more specific input set selection. These bits are defined starting > > + * from the high end of the 64 bits. > > + * Note: If we use above ETH_RSS_* without SRC/DST_ONLY, it > > +represents > > + * both SRC and DST are taken into account. If SRC_ONLY and DST_ONLY > > +of > > + * the same level be used simultaneously, it is the same case as none > > +of > > + * them are added. > > + */ > > +#define ETH_RSS_L3_SRC_ONLY (1ULL << 63) > > +#define ETH_RSS_L3_DST_ONLY (1ULL << 62) > > +#define ETH_RSS_L4_SRC_ONLY (1ULL << 61) > > +#define ETH_RSS_L4_DST_ONLY (1ULL << 60) > > + > > #define ETH_RSS_IP ( \ > > ETH_RSS_IPV4 | \ > > ETH_RSS_FRAG_IPV4 | \
This patch fails to apply to the latest master branch, a rebase may be needed. Regards, Bernard
