> -----Original Message-----
> From: Ilya Maximets <i.maxim...@samsung.com>
> Sent: Thursday, February 21, 2019 5:27 PM
> To: Ophir Munk <ophi...@mellanox.com>; ovs-dev@openvswitch.org
> Cc: Ian Stokes <ian.sto...@intel.com>; Olga Shern <ol...@mellanox.com>;
> Kevin Traynor <ktray...@redhat.com>; Asaf Penso <as...@mellanox.com>;
> Roni Bar Yanai <ron...@mellanox.com>; Flavio Leitner <f...@sysclose.org>
> Subject: Re: [PATCH v2 1/3] netdev-dpdk: Expose flow creation/destruction
> calls
>
> On 21.02.2019 15:07, Ophir Munk wrote:
> > From: Roni Bar Yanai <ron...@mellanox.com>
> >
> > 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 <as...@mellanox.com>
> > Signed-off-by: Roni Bar Yanai <ron...@mellanox.com>
> > Signed-off-by: Ophir Munk <ophi...@mellanox.com>
> > ---
> > 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);
>
Just wondering what will happen if rss action is added with current n_rxq, and
immediately after
there is a reconfiguration. The result will be exactly the same, rss flow has
wrong
configuration. The lock doesn't solve this issue.
> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
