[dpdk-dev] [EXT] Re: [PATCH v16 8/8] app/testpmd: add command to set supported ptype mask

[Ferruh Yigit]

On 11/11/2019 5:02 AM, Jerin Jacob wrote:
> On Mon, Nov 11, 2019 at 10:26 AM Pavan Nikhilesh Bhagavatula
> <pbhagavatula at marvell.com> wrote:
>>
>>> On Fri, Nov 8, 2019 at 7:24 PM Ananyev, Konstantin
>>> <konstantin.ananyev at intel.com> wrote:
>>>>
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: dev <dev-bounces at dpdk.org> On Behalf Of Ferruh Yigit
>>>>> Sent: Thursday, November 7, 2019 7:41 PM
>>>>> To: Jerin Jacob <jerinjacobk at gmail.com>
>>>>> Cc: Pavan Nikhilesh <pbhagavatula at marvell.com>; Andrew
>>> Rybchenko <arybchenko at solarflare.com>; jerinj at marvell.com;
>>>>> thomas at monjalon.net; Lu, Wenzhuo <wenzhuo.lu at intel.com>;
>>> Wu, Jingjing <jingjing.wu at intel.com>; Iremonger, Bernard
>>>>> <bernard.iremonger at intel.com>; Mcnamara, John
>>> <john.mcnamara at intel.com>; Kovacevic, Marko
>>> <marko.kovacevic at intel.com>;
>>>>> dev at 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.yigit at intel.com
>>>>>> <mailto:ferruh.yigit at intel.com>> wrote:
>>>>>>
>>>>>>     On 11/6/2019 7:18 PM, pbhagavatula at marvell.com
>>>>>>     <mailto:pbhagavatula at marvell.com> wrote:
>>>>>>     > From: Pavan Nikhilesh <pbhagavatula at marvell.com
>>>>>>     <mailto:pbhagavatula at 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.