On 11/11/2019 5:02 AM, Jerin Jacob wrote: > On Mon, Nov 11, 2019 at 10:26 AM Pavan Nikhilesh Bhagavatula > <pbhagavat...@marvell.com> wrote: >> >>> On Fri, Nov 8, 2019 at 7:24 PM Ananyev, Konstantin >>> <konstantin.anan...@intel.com> wrote: >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: dev <dev-boun...@dpdk.org> On Behalf Of Ferruh Yigit >>>>> Sent: Thursday, November 7, 2019 7:41 PM >>>>> To: Jerin Jacob <jerinjac...@gmail.com> >>>>> Cc: Pavan Nikhilesh <pbhagavat...@marvell.com>; Andrew >>> Rybchenko <arybche...@solarflare.com>; jer...@marvell.com; >>>>> tho...@monjalon.net; Lu, Wenzhuo <wenzhuo...@intel.com>; >>> Wu, Jingjing <jingjing...@intel.com>; Iremonger, Bernard >>>>> <bernard.iremon...@intel.com>; Mcnamara, John >>> <john.mcnam...@intel.com>; Kovacevic, Marko >>> <marko.kovace...@intel.com>; >>>>> dev@dpdk.org >>>>> Subject: Re: [dpdk-dev] [PATCH v16 8/8] app/testpmd: add >>> command to set supported ptype mask >>>>> >>>>> On 11/7/2019 6:55 PM, Jerin Jacob wrote: >>>>>> >>>>>> >>>>>> On Fri, 8 Nov, 2019, 12:10 am Ferruh Yigit, <ferruh.yi...@intel.com >>>>>> <mailto:ferruh.yi...@intel.com>> wrote: >>>>>> >>>>>> On 11/6/2019 7:18 PM, pbhagavat...@marvell.com >>>>>> <mailto:pbhagavat...@marvell.com> wrote: >>>>>> > From: Pavan Nikhilesh <pbhagavat...@marvell.com >>>>>> <mailto:pbhagavat...@marvell.com>> >>>>>> > >>>>>> > Add command to set supported ptype mask. >>>>>> > Usage: >>>>>> > set port <port_id> ptype_mask > /* *************** >>>>>> > >>>>>> > /* list of instructions */ >>>>>> > @@ -19155,6 +19237,7 @@ cmdline_parse_ctx_t main_ctx[] = >>> { >>>>>> > (cmdline_parse_inst_t *)&cmd_show_vf_stats, >>>>>> > (cmdline_parse_inst_t *)&cmd_clear_vf_stats, >>>>>> > (cmdline_parse_inst_t >>> *)&cmd_show_port_supported_ptypes, >>>>>> > + (cmdline_parse_inst_t *)&cmd_set_port_ptypes, >>>>>> > (cmdline_parse_inst_t *)&cmd_ptype_mapping_get, >>>>>> > (cmdline_parse_inst_t *)&cmd_ptype_mapping_replace, >>>>>> > (cmdline_parse_inst_t *)&cmd_ptype_mapping_reset, >>>>>> > diff --git a/app/test-pmd/testpmd.c b/app/test- >>> pmd/testpmd.c >>>>>> > index 5ba974162..812aebf35 100644 >>>>>> > --- a/app/test-pmd/testpmd.c >>>>>> > +++ b/app/test-pmd/testpmd.c >>>>>> > @@ -2024,6 +2024,7 @@ start_port(portid_t pid) >>>>>> > queueid_t qi; >>>>>> > struct rte_port *port; >>>>>> > struct rte_ether_addr mac_addr; >>>>>> > + static uint8_t clr_ptypes = 1; >>>>>> > >>>>>> > if (port_id_is_invalid(pid, ENABLED_WARN)) >>>>>> > return 0; >>>>>> > @@ -2153,6 +2154,10 @@ start_port(portid_t pid) >>>>>> > } >>>>>> > } >>>>>> > configure_rxtx_dump_callbacks(verbose_level); >>>>>> > + if (clr_ptypes) { >>>>>> > + clr_ptypes = 0; >>>>>> > + rte_eth_dev_set_ptypes(pi, >>> RTE_PTYPE_UNKNOWN, NULL, 0); >>>>>> > + } >>>>>> >>>>>> I am not sure about this command, we have now capability to >>> set/disable ptypes >>>>>> on demand, why disabling them by default? >>>>>> >>>>>> >>>>>> As forward engines are not using the ptype offload. If a specific >>> forwa >>>>>> need the offload, it can be enabled. >>>> >>>> Well, at least now user can see ptype in pkt dump with 'set verbose > >>> 0' >>>> It's definitely looks loke a a behavior change. >>>> Wonder why it can't be done visa-versa? >>>> Keep current behavior as default one (all ptypes are un-masked) and >>>> have special command to disable/enable ptypes. >>>> as I understand such command was added by these series. correct? >>> >>> IMO, we are following the principle that by default all offloads are >>> disabled and enable it >>> based on the need to have optimal performance across the DPDK. This >>> change will be >>> on the same theme. >>> >>> Regarding the verbose > 0 cases, I think, we can call >>> rte_eth_dev_set_ptypes() to all PTYPES on >>> the setting verbose callback. >> >> Agree verbose > 0 case we can do set_ptypes with >> RTE_PTYPE_ALL_MASK. But what if the user wants to verify >> rte_eth_dev_set_ptypes() call itself in verbose mode?. >> Example: >> >> set port 0 ptype_mask RTE_PTYPE_L3_MASK >> set verbose 1 >> >> In this case user expects to see only L3 mask set. Wouldn’t it be confusing >> if >> we set RTE_PTYPE_ALL_MASK under the hood when verbose level is increased? > > I thought of adding RTE_PTYPE_ALL_MASK in the callback to maintain the > existing behavior > if there is any concern in that area. > > Either way is fine with me. (implicit RTE_PTYPE_ALL_MASK in a > callback or explicit mask set in command line) >
I think explicit set in command line is better, setting in the callback can be confusing.
