From: "Iremonger, Bernard" <bernard.iremonger@intel.com>
To: "Yigit, Ferruh" <ferruh.yigit@intel.com>,
"Zhang, Qi Z" <qi.z.zhang@intel.com>,
"Wu, Jingjing" <jingjing.wu@intel.com>,
"Zhang, Helin" <helin.zhang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
Date: Thu, 23 Mar 2017 17:22:47 +0000 [thread overview]
Message-ID: <8CEF83825BEC744B83065625E567D7C224D3870E@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <61684a15-2647-06ff-dcc9-02d030b77b20@intel.com>
Hi Ferruh,
<snip>
> >>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> >>>>>
> >>>>> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> >>>>>> Add a new private API to support the untag drop enable/disable
> >>>>>> for specific VF.
> >>>>>>
> >>>>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >>>>>> ---
> >>>>>> drivers/net/i40e/i40e_ethdev.c | 49
> >>>>>> +++++++++++++++++++++++++++++++++++++++++
> >>>>>> drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
> >>>>>
> >>>>> Shared library is giving build error because of API is missing in
> >>>>> *version.map file
> >>>>>
> >>>>>> 2 files changed, 67 insertions(+)
> >>>>>>
> >>>>>
> >>>>> <...>
> >>>>>
> >>>>>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
> >>>>>> --- a/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> >>>>>> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t
> port,
> >>>>>> int rte_pmd_i40e_reset_vf_stats(uint8_t port,
> >>>>>> uint16_t vf_id);
> >>>>>>
> >>>>>> +/**
> >>>>>> + * Enable/Disable VF untag drop
> >>>>>> + *
> >>>>>> + * @param port
> >>>>>> + * The port identifier of the Ethernet device.
> >>>>>> + * @param vf_id
> >>>>>> + * VF on witch to enable/disable
> >>>>>> + * @param on
> >>>>>> + * Enable or Disable
> >>>>>> + * @retura
> >>>>>
> >>>>> @return
> >>>>>
> >>>>>> + * - (0) if successful.
> >>>>>> + * -(-ENODEVE) if *port* invalid
> >>>>>> + * -(-EINVAL) if bad parameter.
> >>>>>> + */
> >>>>>> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
> >>>>>> + uint16_t vf_id,
> >>>>>> + uint8_t on);
> >>>>>
> >>>>> As discussed previously, I believe it is good to keep following
> >>>>> syntax in
> >>> API:
> >>>>> <name_space>_<object>_<action>, for this API it becomes:
> >>>> I think, current naming rule is <name_space>_<action>_<object>
> right?
> >
> > This seems to be the existing naming convention.
> >
> >>>
> >>> Overall, I am not aware of any defined naming rule, I am for defining
> one.
> >>>
> >>>> See below
> >>>> rte_pmd_i40e_set_vf_vlan_anti_spoof;
> >>>> rte_pmd_i40e_set_vf_vlan_filter;
> >>>> rte_pmd_i40e_set_vf_vlan_insert;
> >>>> rte_pmd_i40e_set_vf_vlan_stripq;
> >>>> rte_pmd_i40e_set_vf_vlan_tag; so what's wrong with this
> >>>
> >>> This breaks hierarchical approach, if you think API name as tree.
> >>> Easier to see this when you sort the APIs, ns_set_x, ns_reset_x,
> >>> ns_del_x will spread to different locations.
> >> I agree with your point, I had thought your concern is only about
> >> this patch, but actually it's not.
> >>>
> >>> This looks OK when you work on one type of object already, but with
> >>> all APIs in concern, I believe object based grouping is better than
> >>> action based grouping.
> >>
> >>>
> >>> And why do you think above one is better? Again, as long as one is
> >>> agreed on,
> >> I don't, sorry for make you misunderstand
> >
> > I don't think changing the name convention at this point is a good idea.
>
> I am not suggesting changing existing ones, for the sake compatibility.
> But that can also be an option, since these are PMD specific API, I expect
> usage will be limited and these does not carry as high standard as library APIs.
These API's are already in use with users.
> > It would be better to remain consistent with the existing naming
> convention.
>
> Existing i40e ones added this way to be compatible with existing ixgbe ones.
> But I don't think we need to follow old usage with new APIs.
>
> > Otherwise both naming conventions will exist for the rte_pmd_i40e_*
> API's.
>
> It can be for a while, later all can be fixed. If you think proposed convention is
> not better, that is something else.
I don't see anything to be gained by using two naming conventions side by side.
I don't have a strong view on which name convention is better, however the existing naming convention is what is in use with users at present. It is also the naming convention used librte_ether.
Changing API naming conventions is something that should probably be discussed in a separate thread, as it can have a wider impact than the i40e and ixgbe PMD's.
> >
> >
> >>>>> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be
> >>> removed?
> >>>>>
> >>>>>> +
> >>>>>> #endif /* _PMD_I40E_H_ */
> >>>>>>
> >>>>
Regards,
Bernard.
next prev parent reply other threads:[~2017-03-23 17:22 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-03 1:59 [dpdk-dev] [PATCH 0/2] " Qi Zhang
2017-03-03 1:59 ` [dpdk-dev] [PATCH 1/2] net/i40e: " Qi Zhang
2017-03-07 10:51 ` Ferruh Yigit
2017-03-09 3:24 ` Zhang, Qi Z
2017-03-14 13:29 ` Ferruh Yigit
2017-03-14 14:32 ` Zhang, Qi Z
2017-03-14 18:16 ` Iremonger, Bernard
2017-03-23 17:00 ` Ferruh Yigit
2017-03-23 17:22 ` Iremonger, Bernard [this message]
2017-03-03 1:59 ` [dpdk-dev] [PATCH 2/2] app/testpmd: enable VF untag drop in testpmd Qi Zhang
2017-03-07 11:13 ` Ferruh Yigit
2017-03-09 2:59 ` Zhang, Qi Z
2017-03-14 13:32 ` Ferruh Yigit
2017-03-14 14:43 ` Zhang, Qi Z
2017-03-13 4:55 ` [dpdk-dev] [PATCH v2 0/2] net/i40e: enable VF untag drop Qi Zhang
2017-03-13 4:55 ` [dpdk-dev] [PATCH v2 1/2] " Qi Zhang
2017-03-13 4:55 ` [dpdk-dev] [PATCH v2 2/2] app/testpmd: enable VF untag drop in testpmd Qi Zhang
2017-03-23 17:01 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8CEF83825BEC744B83065625E567D7C224D3870E@IRSMSX108.ger.corp.intel.com \
--to=bernard.iremonger@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=helin.zhang@intel.com \
--cc=jingjing.wu@intel.com \
--cc=qi.z.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).