| Message ID | A400FC85CF2669428A5A081F01B94F531DA1D4D0@DEMUMBX012.nsn-intra.net |
|---|---|
| State | New |
| Headers | show |
On 31 March 2015 at 09:50, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > > > *From:* ext Bill Fischofer [mailto:bill.fischofer@linaro.org] > *Sent:* Tuesday, March 31, 2015 1:13 AM > *To:* Savolainen, Petri (Nokia - FI/Espoo) > *Cc:* LNG ODP Mailman List > *Subject:* Re: [lng-odp] [RFC 8/8] api: packet_io: renamed > odp_pktio_promisc_mode_set > > > > > > > > On Mon, Mar 30, 2015 at 12:23 PM, Petri Savolainen < > petri.savolainen@nokia.com> wrote: > > Renamed to odp_pktio_ctrl_promisc_mode(). All interface level > control function are prefixed with odp_pktio_ctrl_ to highlight > that these operations may not be permitted on all interfaces. > For example, virtual functions cannot change interface > level settings, only physical functions can. > > Signed-off-by: Petri Savolainen <petri.savolainen@nokia.com> > --- > include/odp/api/packet_io.h | 30 +++++++++++++++++++----------- > platform/linux-generic/odp_packet_io.c | 2 +- > test/validation/odp_pktio.c | 4 ++-- > 3 files changed, 22 insertions(+), 14 deletions(-) > > diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h > index dc76270..3229d64 100644 > --- a/include/odp/api/packet_io.h > +++ b/include/odp/api/packet_io.h > @@ -317,17 +317,6 @@ int odp_pktio_ctrl_info(odp_pktio_t pktio, > odp_pktio_ctrl_info_t *info); > int odp_pktio_mtu(odp_pktio_t pktio); > > /** > - * Enable/Disable promiscuous mode on a packet IO interface. > - * > - * @param[in] pktio Packet IO handle. > - * @param[in] enable 1 to enable, 0 to disable. > - * > - * @retval 0 on success > - * @retval <0 on failure > - */ > -int odp_pktio_promisc_mode_set(odp_pktio_t pktio, odp_bool_t enable); > - > -/** > * Determine if promiscuous mode is enabled for a packet IO interface. > * > * @param[in] pktio Packet IO handle. > @@ -420,6 +409,25 @@ int odp_pktio_headroom_set(odp_pktio_t pktio, > uint32_t headroom); > */ > uint64_t odp_pktio_to_u64(odp_pktio_t pktio); > > +/* > + * Packet IO interface control > + * --------------------------- > + * > + * Some functions may not be permitted on all interfaces > + */ > + > +/** > + * Enable/Disable promiscuous mode on a packet IO interface. > + * > + * @param[in] pktio Packet IO handle. > + * @param[in] enable 1 to enable, 0 to disable. > + * > + * @retval 0 on success > + * @retval <0 on failure > + */ > +int odp_pktio_ctrl_promisc_mode(odp_pktio_t pktio, odp_bool_t enable); > > > > This seems inconsistent with other getters/setters used throughout ODP. > Why should these attributes have their own unique syntax instead of > odp_pktio_promisc_mode() for the getter and odp_pktio_promisc_mode_set() > for setter? > > > > The odp_pktio_ctrl_ API would be used to modifying the shared state of the > interface (e.g. link up/down, or enable/disable promisc mode). Depending on > the interface sharing (e.g. VF vs. PF), some of the API calls may not be > supported on the interface the user has opened (a VF). User can call > odp_pktio_ctrl_info() to check which ctrl API functions are permitted. > > > > odp_pktio_ctrl_info(pktio, &ctrl_info); > > > > if (ctrl_info.promisc == 0) { > > printf(“No permission to set promisc mode\n”); > > return; > > } > > > > if (odp_pktio_ctrl_promisc_mode(pktio, 1)) > > printf(“Promisc mode set failed\n”); > > > > > > VS. > > > > if (odp_pktio_ctrl_promisc_mode(pktio, 1)) > > printf(“No permission to set promisc mode, or set failed\n”); > > > > > > This way the potentially not supported functions are grouped together. > Other option would be to standadise the return value (-1 fail, -2 not > supported) or use errno (-1 fail, errno== ENOSUPPORT). But I think it’s > better to group and highlight this class of functions. All odp_xxx_set() > functions would remain “supported” always. > You are breaking backwards compatibility for some subjective feature that functions that under certain circumstances will or can fail will be grouped together. And I cannot really see the relationship between VF (virtual function) and "ctrl" function naming in the API. Are we even sure that this convention will be possible to uphold? Some HW may break it and then the naming convention will be meaningless. Better to have a return code that signifies that the operation on a shared resource (e.g. virtual function) is not possible. > > > -Petri > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp > >
We also want to be careful about defining aggregates as part of the ODP APIs. However "natural" an aggregate may seem to us, we can be sure they will map awkwardly to some implementations where the "related" information is in fact widely scattered and would need to be assembled. Better to provide individual attribute controls and let the implementation perform behind-the-scenes caching as needed if performance is a concern. On Tue, Mar 31, 2015 at 6:34 AM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 31 March 2015 at 09:50, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > >> >> >> >> >> *From:* ext Bill Fischofer [mailto:bill.fischofer@linaro.org] >> *Sent:* Tuesday, March 31, 2015 1:13 AM >> *To:* Savolainen, Petri (Nokia - FI/Espoo) >> *Cc:* LNG ODP Mailman List >> *Subject:* Re: [lng-odp] [RFC 8/8] api: packet_io: renamed >> odp_pktio_promisc_mode_set >> >> >> >> >> >> >> >> On Mon, Mar 30, 2015 at 12:23 PM, Petri Savolainen < >> petri.savolainen@nokia.com> wrote: >> >> Renamed to odp_pktio_ctrl_promisc_mode(). All interface level >> control function are prefixed with odp_pktio_ctrl_ to highlight >> that these operations may not be permitted on all interfaces. >> For example, virtual functions cannot change interface >> level settings, only physical functions can. >> >> Signed-off-by: Petri Savolainen <petri.savolainen@nokia.com> >> --- >> include/odp/api/packet_io.h | 30 >> +++++++++++++++++++----------- >> platform/linux-generic/odp_packet_io.c | 2 +- >> test/validation/odp_pktio.c | 4 ++-- >> 3 files changed, 22 insertions(+), 14 deletions(-) >> >> diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h >> index dc76270..3229d64 100644 >> --- a/include/odp/api/packet_io.h >> +++ b/include/odp/api/packet_io.h >> @@ -317,17 +317,6 @@ int odp_pktio_ctrl_info(odp_pktio_t pktio, >> odp_pktio_ctrl_info_t *info); >> int odp_pktio_mtu(odp_pktio_t pktio); >> >> /** >> - * Enable/Disable promiscuous mode on a packet IO interface. >> - * >> - * @param[in] pktio Packet IO handle. >> - * @param[in] enable 1 to enable, 0 to disable. >> - * >> - * @retval 0 on success >> - * @retval <0 on failure >> - */ >> -int odp_pktio_promisc_mode_set(odp_pktio_t pktio, odp_bool_t enable); >> - >> -/** >> * Determine if promiscuous mode is enabled for a packet IO interface. >> * >> * @param[in] pktio Packet IO handle. >> @@ -420,6 +409,25 @@ int odp_pktio_headroom_set(odp_pktio_t pktio, >> uint32_t headroom); >> */ >> uint64_t odp_pktio_to_u64(odp_pktio_t pktio); >> >> +/* >> + * Packet IO interface control >> + * --------------------------- >> + * >> + * Some functions may not be permitted on all interfaces >> + */ >> + >> +/** >> + * Enable/Disable promiscuous mode on a packet IO interface. >> + * >> + * @param[in] pktio Packet IO handle. >> + * @param[in] enable 1 to enable, 0 to disable. >> + * >> + * @retval 0 on success >> + * @retval <0 on failure >> + */ >> +int odp_pktio_ctrl_promisc_mode(odp_pktio_t pktio, odp_bool_t enable); >> >> >> >> This seems inconsistent with other getters/setters used throughout ODP. >> Why should these attributes have their own unique syntax instead of >> odp_pktio_promisc_mode() for the getter and odp_pktio_promisc_mode_set() >> for setter? >> >> >> >> The odp_pktio_ctrl_ API would be used to modifying the shared state of >> the interface (e.g. link up/down, or enable/disable promisc mode). >> Depending on the interface sharing (e.g. VF vs. PF), some of the API calls >> may not be supported on the interface the user has opened (a VF). User can >> call odp_pktio_ctrl_info() to check which ctrl API functions are permitted. >> >> >> >> odp_pktio_ctrl_info(pktio, &ctrl_info); >> >> >> >> if (ctrl_info.promisc == 0) { >> >> printf(“No permission to set promisc mode\n”); >> >> return; >> >> } >> >> >> >> if (odp_pktio_ctrl_promisc_mode(pktio, 1)) >> >> printf(“Promisc mode set failed\n”); >> >> >> >> >> >> VS. >> >> >> >> if (odp_pktio_ctrl_promisc_mode(pktio, 1)) >> >> printf(“No permission to set promisc mode, or set failed\n”); >> >> >> >> >> >> This way the potentially not supported functions are grouped together. >> Other option would be to standadise the return value (-1 fail, -2 not >> supported) or use errno (-1 fail, errno== ENOSUPPORT). But I think it’s >> better to group and highlight this class of functions. All odp_xxx_set() >> functions would remain “supported” always. >> > You are breaking backwards compatibility for some subjective feature that > functions that under certain circumstances will or can fail will be grouped > together. And I cannot really see the relationship between VF (virtual > function) and "ctrl" function naming in the API. > > Are we even sure that this convention will be possible to uphold? Some HW > may break it and then the naming convention will be meaningless. > > Better to have a return code that signifies that the operation on a shared > resource (e.g. virtual function) is not possible. > > >> >> >> -Petri >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >
diff --git a/include/odp/api/packet_io.h b/include/odp/api/packet_io.h index dc76270..3229d64 100644 --- a/include/odp/api/packet_io.h +++ b/include/odp/api/packet_io.h @@ -317,17 +317,6 @@ int odp_pktio_ctrl_info(odp_pktio_t pktio, odp_pktio_ctrl_info_t *info); int odp_pktio_mtu(odp_pktio_t pktio); /** - * Enable/Disable promiscuous mode on a packet IO interface. - * - * @param[in] pktio Packet IO handle. - * @param[in] enable 1 to enable, 0 to disable. - * - * @retval 0 on success - * @retval <0 on failure - */ -int odp_pktio_promisc_mode_set(odp_pktio_t pktio, odp_bool_t enable); - -/** * Determine if promiscuous mode is enabled for a packet IO interface. * * @param[in] pktio Packet IO handle. @@ -420,6 +409,25 @@ int odp_pktio_headroom_set(odp_pktio_t pktio, uint32_t headroom); */ uint64_t odp_pktio_to_u64(odp_pktio_t pktio); +/* + * Packet IO interface control + * --------------------------- + * + * Some functions may not be permitted on all interfaces + */ + +/** + * Enable/Disable promiscuous mode on a packet IO interface. + * + * @param[in] pktio Packet IO handle. + * @param[in] enable 1 to enable, 0 to disable. + * + * @retval 0 on success + * @retval <0 on failure + */ +int odp_pktio_ctrl_promisc_mode(odp_pktio_t pktio, odp_bool_t enable); This seems inconsistent with other getters/setters used throughout ODP. Why should these attributes have their own unique syntax instead of odp_pktio_promisc_mode() for the getter and odp_pktio_promisc_mode_set() for setter? The odp_pktio_ctrl_ API would be used to modifying the shared state of the interface (e.g. link up/down, or enable/disable promisc mode). Depending on the interface sharing (e.g. VF vs. PF), some of the API calls may not be supported on the interface the user has opened (a VF). User can call odp_pktio_ctrl_info() to check which ctrl API functions are permitted.