| Message ID | 1417110685-2931-3-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 November 2014 at 18:51, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Define API and implement promisc functions for linux-generic. > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/include/api/odp_packet_io.h | 24 +++++++ > platform/linux-generic/odp_packet_io.c | 74 ++++++++++++++++++++++ > 2 files changed, 98 insertions(+) > > diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h > index 667395c..c7d1e40 100644 > --- a/platform/linux-generic/include/api/odp_packet_io.h > +++ b/platform/linux-generic/include/api/odp_packet_io.h > @@ -149,6 +149,30 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); > int odp_pktio_mtu(odp_pktio_t id); > > /** > + * Enable promiscuous mode on a packet IO interface. > + * > + * @param[in] id ODP packet IO handle. > + * @param[in] enable 1 enabled, 0 disabled. > + * > + * @retval 0 on success. > + * @retval -1 on a bad pktio id > + * @retval -1 any other error > + */ > +int odp_pktio_promisc_set(odp_pktio_t id, odp_bool_t enable); > + > +/** > + * Determine if promiscuous mode is enabled for a packet IO interface. > + * > + * @param[in] id ODP packet IO handle. > + * > + * @retval 1 if promiscuous mode is enabled. > + * @retval 0 if promiscuous mode is disabled. > + * @retval -1 on a bad pktio id > + * @retval -1 any other error > +*/ > +int odp_pktio_promisc_enabled(odp_pktio_t id); > + > +/** > * @} > */ > > diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c > index c523350..9140620 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -542,3 +542,77 @@ int odp_pktio_mtu(odp_pktio_t id) > > return ifr.ifr_mtu; > } > + > +int odp_pktio_promisc_set(odp_pktio_t id, odp_bool_t enable) > +{ > + pktio_entry_t *entry; > + int sockfd; > + struct ifreq ifr; > + int ret; > + > + entry = get_entry(id); > + if (entry == NULL) { > + ODP_DBG("pktio entry %d does not exist\n", id); > + return -1; > + } > + > + if (entry->s.pkt_sock_mmap.sockfd) > + sockfd = entry->s.pkt_sock_mmap.sockfd; > + else > + sockfd = entry->s.pkt_sock.sockfd; > + > + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1); > + ifr.ifr_name[IFNAMSIZ] = 0; > + > + ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr); > + if (ret < 0) { > + ODP_DBG("ioctl SIOCGIFFLAGS error\n"); > + return -1; > + } > + > + if (enable) > + ifr.ifr_flags |= IFF_PROMISC; > + else > + ifr.ifr_flags &= ~(IFF_PROMISC); As some people are concerned with branching caused by different values of the enable parameter, we can express this in a branch-less way: #define UPDATE(old, mask, ena) ((old) & ~(mask)) | (((ena) ? ~0U : 0) & (mask)) ifr.ifr_flags = UPDATE(ifr.ifr_flags, IFF_PROMISC, enable); GCC for ARMv7 generates pretty compact code for this, no branches (uses on ITE (if-then-else) instruction though). Branch-less code is preferred if the branches are difficult to predict (e.g. more or less random). But don't overdo it since modern branch predictors are quite good. > + > + ret = ioctl(sockfd, SIOCSIFFLAGS, &ifr); > + if (ret < 0) { > + ODP_DBG("ioctl SIOCSIFFLAGS error\n"); > + return -1; > + } > + > + return 0; > +} > + > +int odp_pktio_promisc_enabled(odp_pktio_t id) > +{ > + pktio_entry_t *entry; > + int sockfd; > + struct ifreq ifr; > + int ret; > + > + entry = get_entry(id); > + if (entry == NULL) { > + ODP_DBG("pktio entry %d does not exist\n", id); > + return -1; > + } > + > + if (entry->s.pkt_sock_mmap.sockfd) > + sockfd = entry->s.pkt_sock_mmap.sockfd; > + else > + sockfd = entry->s.pkt_sock.sockfd; > + > + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1); > + ifr.ifr_name[IFNAMSIZ] = 0; > + > + ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr); > + if (ret < 0) { > + ODP_DBG("ioctl SIOCGIFFLAGS error\n"); > + return -1; > + } > + > + if (ifr.ifr_flags & IFF_PROMISC) > + return 1; > + else > + return 0; > +} > -- > 1.8.5.1.163.gd7aced9 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
Worrying about the performance of functions like promiscuous enabling is misplaced since this routine would hardly be used in a performance path. The real issue is the syntax. Has Petri defined what he wants for these yet? This seems like a simply attribute on the API odp_pktio_t object. Either it is or is not in promiscuous mode. As such, to be consistent with similar attributes one would expect the getter to be: odp_bool_t odp_pktio_promisc(odp_pktio_t io); and the setter to be: void odp_pktio_set_promisc(odp_pktio_t io, odp_bool_t truefalse); Bill On Thu, Nov 27, 2014 at 3:12 PM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 27 November 2014 at 18:51, Maxim Uvarov <maxim.uvarov@linaro.org> > wrote: > > Define API and implement promisc functions for linux-generic. > > > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > > --- > > platform/linux-generic/include/api/odp_packet_io.h | 24 +++++++ > > platform/linux-generic/odp_packet_io.c | 74 > ++++++++++++++++++++++ > > 2 files changed, 98 insertions(+) > > > > diff --git a/platform/linux-generic/include/api/odp_packet_io.h > b/platform/linux-generic/include/api/odp_packet_io.h > > index 667395c..c7d1e40 100644 > > --- a/platform/linux-generic/include/api/odp_packet_io.h > > +++ b/platform/linux-generic/include/api/odp_packet_io.h > > @@ -149,6 +149,30 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); > > int odp_pktio_mtu(odp_pktio_t id); > > > > /** > > + * Enable promiscuous mode on a packet IO interface. > > + * > > + * @param[in] id ODP packet IO handle. > > + * @param[in] enable 1 enabled, 0 disabled. > > + * > > + * @retval 0 on success. > > + * @retval -1 on a bad pktio id > > + * @retval -1 any other error > > + */ > > +int odp_pktio_promisc_set(odp_pktio_t id, odp_bool_t enable); > > + > > +/** > > + * Determine if promiscuous mode is enabled for a packet IO interface. > > + * > > + * @param[in] id ODP packet IO handle. > > + * > > + * @retval 1 if promiscuous mode is enabled. > > + * @retval 0 if promiscuous mode is disabled. > > + * @retval -1 on a bad pktio id > > + * @retval -1 any other error > > +*/ > > +int odp_pktio_promisc_enabled(odp_pktio_t id); > > + > > +/** > > * @} > > */ > > > > diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > > index c523350..9140620 100644 > > --- a/platform/linux-generic/odp_packet_io.c > > +++ b/platform/linux-generic/odp_packet_io.c > > @@ -542,3 +542,77 @@ int odp_pktio_mtu(odp_pktio_t id) > > > > return ifr.ifr_mtu; > > } > > + > > +int odp_pktio_promisc_set(odp_pktio_t id, odp_bool_t enable) > > +{ > > + pktio_entry_t *entry; > > + int sockfd; > > + struct ifreq ifr; > > + int ret; > > + > > + entry = get_entry(id); > > + if (entry == NULL) { > > + ODP_DBG("pktio entry %d does not exist\n", id); > > + return -1; > > + } > > + > > + if (entry->s.pkt_sock_mmap.sockfd) > > + sockfd = entry->s.pkt_sock_mmap.sockfd; > > + else > > + sockfd = entry->s.pkt_sock.sockfd; > > + > > + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1); > > + ifr.ifr_name[IFNAMSIZ] = 0; > > + > > + ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr); > > + if (ret < 0) { > > + ODP_DBG("ioctl SIOCGIFFLAGS error\n"); > > + return -1; > > + } > > + > > + if (enable) > > + ifr.ifr_flags |= IFF_PROMISC; > > + else > > + ifr.ifr_flags &= ~(IFF_PROMISC); > As some people are concerned with branching caused by different values > of the enable parameter, we can express this in a branch-less way: > #define UPDATE(old, mask, ena) ((old) & ~(mask)) | (((ena) ? ~0U : 0) & > (mask)) > ifr.ifr_flags = UPDATE(ifr.ifr_flags, IFF_PROMISC, enable); > > GCC for ARMv7 generates pretty compact code for this, no branches > (uses on ITE (if-then-else) instruction though). > > Branch-less code is preferred if the branches are difficult to predict > (e.g. more or less random). But don't overdo it since modern branch > predictors are quite good. > > > > + > > + ret = ioctl(sockfd, SIOCSIFFLAGS, &ifr); > > + if (ret < 0) { > > + ODP_DBG("ioctl SIOCSIFFLAGS error\n"); > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +int odp_pktio_promisc_enabled(odp_pktio_t id) > > +{ > > + pktio_entry_t *entry; > > + int sockfd; > > + struct ifreq ifr; > > + int ret; > > + > > + entry = get_entry(id); > > + if (entry == NULL) { > > + ODP_DBG("pktio entry %d does not exist\n", id); > > + return -1; > > + } > > + > > + if (entry->s.pkt_sock_mmap.sockfd) > > + sockfd = entry->s.pkt_sock_mmap.sockfd; > > + else > > + sockfd = entry->s.pkt_sock.sockfd; > > + > > + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1); > > + ifr.ifr_name[IFNAMSIZ] = 0; > > + > > + ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr); > > + if (ret < 0) { > > + ODP_DBG("ioctl SIOCGIFFLAGS error\n"); > > + return -1; > > + } > > + > > + if (ifr.ifr_flags & IFF_PROMISC) > > + return 1; > > + else > > + return 0; > > +} > > -- > > 1.8.5.1.163.gd7aced9 > > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 11/27/2014 10:05 PM, Stuart Haslam wrote: > On Thu, Nov 27, 2014 at 05:51:19PM +0000, Maxim Uvarov wrote: >> Define API and implement promisc functions for linux-generic. >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> --- >> platform/linux-generic/include/api/odp_packet_io.h | 24 +++++++ >> platform/linux-generic/odp_packet_io.c | 74 ++++++++++++++++++++++ >> 2 files changed, 98 insertions(+) >> >> diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h >> index 667395c..c7d1e40 100644 >> --- a/platform/linux-generic/include/api/odp_packet_io.h >> +++ b/platform/linux-generic/include/api/odp_packet_io.h >> @@ -149,6 +149,30 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); >> int odp_pktio_mtu(odp_pktio_t id); >> >> /** >> + * Enable promiscuous mode on a packet IO interface. >> + * >> + * @param[in] id ODP packet IO handle. >> + * @param[in] enable 1 enabled, 0 disabled. >> + * >> + * @retval 0 on success. >> + * @retval -1 on a bad pktio id >> + * @retval -1 any other error >> + */ >> +int odp_pktio_promisc_set(odp_pktio_t id, odp_bool_t enable); >> + >> +/** >> + * Determine if promiscuous mode is enabled for a packet IO interface. >> + * >> + * @param[in] id ODP packet IO handle. >> + * >> + * @retval 1 if promiscuous mode is enabled. >> + * @retval 0 if promiscuous mode is disabled. >> + * @retval -1 on a bad pktio id >> + * @retval -1 any other error >> +*/ >> +int odp_pktio_promisc_enabled(odp_pktio_t id); >> + >> +/** >> * @} >> */ >> >> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c >> index c523350..9140620 100644 >> --- a/platform/linux-generic/odp_packet_io.c >> +++ b/platform/linux-generic/odp_packet_io.c >> @@ -542,3 +542,77 @@ int odp_pktio_mtu(odp_pktio_t id) >> >> return ifr.ifr_mtu; >> } >> + >> +int odp_pktio_promisc_set(odp_pktio_t id, odp_bool_t enable) >> +{ >> + pktio_entry_t *entry; >> + int sockfd; >> + struct ifreq ifr; >> + int ret; >> + >> + entry = get_entry(id); >> + if (entry == NULL) { >> + ODP_DBG("pktio entry %d does not exist\n", id); >> + return -1; >> + } >> + >> + if (entry->s.pkt_sock_mmap.sockfd) > This isn't right, 0 is a valid value for a sockfd, Aha, I didn't know that. If stdin is closed, than socket() really can return 0 and it's valid fd. I see that you fixed that in other patch. Thanks. > although you can't > check against -1 here either as the structure is initialised to 0. > You'll either need to initialise the sockfds properly or do what is done > elsewhere and check the entry->s.type Yep. Maxim. > > The odp_pktio_set_mtu and odp_pktio_mtu functions have the same issue. > >> + sockfd = entry->s.pkt_sock_mmap.sockfd; >> + else >> + sockfd = entry->s.pkt_sock.sockfd; >> + >> + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1); >> + ifr.ifr_name[IFNAMSIZ] = 0; > This overruns, need to use IFNAMSIZ-1 >
diff --git a/platform/linux-generic/include/api/odp_packet_io.h b/platform/linux-generic/include/api/odp_packet_io.h index 667395c..c7d1e40 100644 --- a/platform/linux-generic/include/api/odp_packet_io.h +++ b/platform/linux-generic/include/api/odp_packet_io.h @@ -149,6 +149,30 @@ int odp_pktio_set_mtu(odp_pktio_t id, int mtu); int odp_pktio_mtu(odp_pktio_t id); /** + * Enable promiscuous mode on a packet IO interface. + * + * @param[in] id ODP packet IO handle. + * @param[in] enable 1 enabled, 0 disabled. + * + * @retval 0 on success. + * @retval -1 on a bad pktio id + * @retval -1 any other error + */ +int odp_pktio_promisc_set(odp_pktio_t id, odp_bool_t enable); + +/** + * Determine if promiscuous mode is enabled for a packet IO interface. + * + * @param[in] id ODP packet IO handle. + * + * @retval 1 if promiscuous mode is enabled. + * @retval 0 if promiscuous mode is disabled. + * @retval -1 on a bad pktio id + * @retval -1 any other error +*/ +int odp_pktio_promisc_enabled(odp_pktio_t id); + +/** * @} */ diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c index c523350..9140620 100644 --- a/platform/linux-generic/odp_packet_io.c +++ b/platform/linux-generic/odp_packet_io.c @@ -542,3 +542,77 @@ int odp_pktio_mtu(odp_pktio_t id) return ifr.ifr_mtu; } + +int odp_pktio_promisc_set(odp_pktio_t id, odp_bool_t enable) +{ + pktio_entry_t *entry; + int sockfd; + struct ifreq ifr; + int ret; + + entry = get_entry(id); + if (entry == NULL) { + ODP_DBG("pktio entry %d does not exist\n", id); + return -1; + } + + if (entry->s.pkt_sock_mmap.sockfd) + sockfd = entry->s.pkt_sock_mmap.sockfd; + else + sockfd = entry->s.pkt_sock.sockfd; + + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1); + ifr.ifr_name[IFNAMSIZ] = 0; + + ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr); + if (ret < 0) { + ODP_DBG("ioctl SIOCGIFFLAGS error\n"); + return -1; + } + + if (enable) + ifr.ifr_flags |= IFF_PROMISC; + else + ifr.ifr_flags &= ~(IFF_PROMISC); + + ret = ioctl(sockfd, SIOCSIFFLAGS, &ifr); + if (ret < 0) { + ODP_DBG("ioctl SIOCSIFFLAGS error\n"); + return -1; + } + + return 0; +} + +int odp_pktio_promisc_enabled(odp_pktio_t id) +{ + pktio_entry_t *entry; + int sockfd; + struct ifreq ifr; + int ret; + + entry = get_entry(id); + if (entry == NULL) { + ODP_DBG("pktio entry %d does not exist\n", id); + return -1; + } + + if (entry->s.pkt_sock_mmap.sockfd) + sockfd = entry->s.pkt_sock_mmap.sockfd; + else + sockfd = entry->s.pkt_sock.sockfd; + + strncpy(ifr.ifr_name, entry->s.name, IFNAMSIZ - 1); + ifr.ifr_name[IFNAMSIZ] = 0; + + ret = ioctl(sockfd, SIOCGIFFLAGS, &ifr); + if (ret < 0) { + ODP_DBG("ioctl SIOCGIFFLAGS error\n"); + return -1; + } + + if (ifr.ifr_flags & IFF_PROMISC) + return 1; + else + return 0; +}
Define API and implement promisc functions for linux-generic. Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/include/api/odp_packet_io.h | 24 +++++++ platform/linux-generic/odp_packet_io.c | 74 ++++++++++++++++++++++ 2 files changed, 98 insertions(+)