On 21.02.2019 15:07, Ophir Munk wrote: > From: Roni Bar Yanai <[email protected]> > > Before offloading-code was added to the netdev-dpdk.c file (MARK and > RSS actions) the only DPDK RTE calls in use were rte_flow_create() and > rte_flow_destroy(). In preparation for splitting the offloading-code > from the netdev-dpdk.c file to a separate file, it is required > to embed these RTE calls into a global netdev-dpdk-* API so that > they can be called from the new file. An example for this requirement > can be seen in the handling of dpdk_mutex, which should be encapsulated
You probably meant dev->mutex. > inside netdev-dpdk class (netdev-dpdk.c file), and should be unknown > to the outside callers. This commit embeds the rte_flow_create() call > inside the netdev_dpdk_flow_create() API and the rte_flow_destroy() > call inside the netdev_dpdk_rte_flow_destroy() API. > > Reviewed-by: Asaf Penso <[email protected]> > Signed-off-by: Roni Bar Yanai <[email protected]> > Signed-off-by: Ophir Munk <[email protected]> > --- > lib/netdev-dpdk.c | 51 +++++++++++++++++++++++++++++++++++++++------------ > lib/netdev-dpdk.h | 14 ++++++++++++++ > 2 files changed, 53 insertions(+), 12 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 32a6ffd..163d4ec 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -4203,6 +4203,42 @@ unlock: > return err; > } > > +int > +netdev_dpdk_rte_flow_destroy(struct netdev *netdev, > + struct rte_flow *rte_flow, > + struct rte_flow_error *error) > +{ > + if (!is_dpdk_class(netdev->netdev_class)) { > + return -1; > + } Not sure if this check is needed, because we're registering this offload API only for DPDK devices. However, it's not correct anyway, because 'is_dpdk_class' will return 'true' for vhost netdevs which are not rte_ethdev devices, i.e. has no offloading API. > + > + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > + int ret; > + > + ovs_mutex_lock(&dev->mutex); > + ret = rte_flow_destroy(dev->port_id, rte_flow, error); > + ovs_mutex_unlock(&dev->mutex); > + return ret; > +} > + > +struct rte_flow * > +netdev_dpdk_rte_flow_create(struct netdev *netdev, > + const struct rte_flow_attr *attr, > + const struct rte_flow_item *items, > + const struct rte_flow_action *actions, > + struct rte_flow_error *error) > +{ > + if (!is_dpdk_class(netdev->netdev_class)) { > + return NULL; > + } Same here. > + > + struct rte_flow *flow; > + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); It's better to have an empty line between declarations and code. > + ovs_mutex_lock(&dev->mutex); > + flow = rte_flow_create(dev->port_id, attr, items, actions, error); > + ovs_mutex_unlock(&dev->mutex); > + return flow; > +} > > /* Find rte_flow with @ufid */ > static struct rte_flow * > @@ -4554,7 +4590,6 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev, > size_t actions_len OVS_UNUSED, > const ovs_u128 *ufid, > struct offload_info *info) { > - struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > const struct rte_flow_attr flow_attr = { > .group = 0, > .priority = 0, > @@ -4759,15 +4794,12 @@ end_proto_check: > mark.id = info->flow_mark; > add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_MARK, &mark); > > - ovs_mutex_lock(&dev->mutex); > > rss = add_flow_rss_action(&actions, netdev); This doesn't look right. 'add_flow_rss_action' Uses 'netdev->n_rxq' that should not be accessed without 'dev->mutex'. Otherwise you may mess up with reconfiguration and segfault here. This could be avoided if dpif-netdev will really wait for completion of all the offloading operations for the device before reconfiguring it, but this is not yet implemented and will probably require changes in netdev offloading API. And this is, probably, one of the examples of atomicity, but it's not the atomicity of few subsequent rte_flow calls, but the atomicity of work with netdev fields. Because even if this will not crash, we'll try to offload rss action with incorrect number of queues. > add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_END, NULL); > > - flow = rte_flow_create(dev->port_id, &flow_attr, patterns.items, > - actions.actions, &error); > - > - ovs_mutex_unlock(&dev->mutex); > + flow = netdev_dpdk_rte_flow_create(netdev, &flow_attr,patterns.items, > + actions.actions, &error); Please, keep the indents for function arguments. i.e. it's better to align arguments to the first parenthesis. This comment is for many other places in code. > > free(rss); > if (!flow) { > @@ -4861,13 +4893,9 @@ static int > netdev_dpdk_destroy_rte_flow(struct netdev *netdev, > const ovs_u128 *ufid, > struct rte_flow *rte_flow) { > - struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > struct rte_flow_error error; > - int ret; > + int ret = netdev_dpdk_rte_flow_destroy(netdev, rte_flow, &error); > > - ovs_mutex_lock(&dev->mutex); > - > - ret = rte_flow_destroy(dev->port_id, rte_flow, &error); > if (ret == 0) { > ufid_to_rte_flow_disassociate(ufid); > VLOG_DBG("%s: removed rte flow %p associated with ufid " UUID_FMT > "\n", > @@ -4878,7 +4906,6 @@ netdev_dpdk_destroy_rte_flow(struct netdev *netdev, > netdev_get_name(netdev), error.type, error.message); > } > > - ovs_mutex_unlock(&dev->mutex); > return ret; > } > > diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h > index b7d02a7..82d2828 100644 > --- a/lib/netdev-dpdk.h > +++ b/lib/netdev-dpdk.h > @@ -22,11 +22,25 @@ > #include "openvswitch/compiler.h" > > struct dp_packet; > +struct netdev; > +struct rte_flow; > +struct rte_flow_error; > +struct rte_flow_attr; > +struct rte_flow_item; > +struct rte_flow_action; > > #ifdef DPDK_NETDEV > > void netdev_dpdk_register(void); > void free_dpdk_buf(struct dp_packet *); > +int netdev_dpdk_rte_flow_destroy(struct netdev *netdev, > + struct rte_flow *rte_flow, > + struct rte_flow_error *error); > +struct rte_flow *netdev_dpdk_rte_flow_create(struct netdev *netdev, > + const struct rte_flow_attr *attr, > + const struct rte_flow_item *items, > + const struct rte_flow_action *actions, > + struct rte_flow_error *error); Alignments. > > #else > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
