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)
