[ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow

Simon Horman simon.horman at netronome.com
Wed Sep 13 09:45:30 UTC 2017


On Tue, Sep 12, 2017 at 08:34:44AM +0000, Darrell Ball wrote:
> 
> 
> On 9/10/17, 11:11 PM, "ovs-dev-bounces at openvswitch.org on behalf of Yuanhan Liu" <ovs-dev-bounces at openvswitch.org on behalf of yliu at fridaylinux.org> wrote:
> 
>     On Fri, Sep 08, 2017 at 06:47:55PM +0200, Simon Horman wrote:
>     > On Tue, Sep 05, 2017 at 05:22:57PM +0800, Yuanhan Liu wrote:
>     > > From: Finn Christensen <fc at napatech.com>
>     > > 
>     > > The basic yet the major part of this patch is to translate the "match"
>     > > to rte flow patterns. And then, we create a rte flow with a MARK action.
>     > > Afterwards, all pkts matches the flow will have the mark id in the mbuf.
>     > > 
>     > > For any unsupported flows, such as MPLS, -1 is returned, meaning the
>     > > flow offload is failed and then skipped.
>     > 
>     > ...
>     > 
>     > > +static int
>     > > +netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>     > > +                                 const struct match *match,
>     > > +                                 struct nlattr *nl_actions OVS_UNUSED,
>     > > +                                 size_t actions_len,
>     > > +                                 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,
>     > > +        .ingress = 1,
>     > > +        .egress = 0
>     > > +    };
>     > > +    struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
>     > > +    struct flow_actions actions = { .actions = NULL, .cnt = 0 };
>     > 
>     > I believe the following will give the same result as the above,
>     > less verbosely.
>     > 
>     > +    const struct rte_flow_attr flow_attr = { .ingress = 1 };
>     > +    struct flow_patterns patterns = { .cnt = 0 };
>     > +    struct flow_actions actions = { .cnt = 0 };
>     
>     I'm not quite sure on that. If the compiler doesn't do zero assigment
>     correctly, something deadly wrong could happen. They all are short
>     structs, that I think it's fine, IMO. If they are big, I'd prefer an
>     explicit memset.

I'm pretty sure that the compiler will zero all other fields when the
scheme I described above is used. However, its a moot point if the approach
you used is preferred. So I think what you have is fine.

>     
>     > > +    struct rte_flow *flow;
>     > > +    struct rte_flow_error error;
>     > > +    uint8_t *ipv4_next_proto_mask = NULL;
>     > > +    int ret = 0;
>     > > +
>     > > +    /* Eth */
>     > > +    struct rte_flow_item_eth eth_spec;
>     > > +    struct rte_flow_item_eth eth_mask;
>     > 
>     > Empty line here please.
>     
>     I actually want to bind the two declartions with the following code piece,
>     to show that they are tightly related.

As I think Darrell pointed out elsewhere it is common practice to
put declarations close to code that uses them. This is different from
my assumption that they should be grouped at the top of a context.
With this in mind what you already have makes sense to me.

>     
>     > > +    memset(&eth_mask, 0, sizeof(eth_mask));
>     > > +    if (match->wc.masks.dl_src.be16[0] ||
>     > > +        match->wc.masks.dl_src.be16[1] ||
>     > > +        match->wc.masks.dl_src.be16[2] ||
>     > > +        match->wc.masks.dl_dst.be16[0] ||
>     > > +        match->wc.masks.dl_dst.be16[1] ||
>     > > +        match->wc.masks.dl_dst.be16[2]) {
>     > 
>     > It looks like eth_addr_is_zero() can be used in the above.
>     
>     Yes, we could. Thanks for the tip.
>     
>     > > +        rte_memcpy(&eth_spec.dst, &match->flow.dl_dst, sizeof(eth_spec.dst));
>     > > +        rte_memcpy(&eth_spec.src, &match->flow.dl_src, sizeof(eth_spec.src));
>     > > +        eth_spec.type = match->flow.dl_type;
>     > > +
>     > > +        rte_memcpy(&eth_mask.dst, &match->wc.masks.dl_dst,
>     > > +                   sizeof(eth_mask.dst));
>     > > +        rte_memcpy(&eth_mask.src, &match->wc.masks.dl_src,
>     > > +                   sizeof(eth_mask.src));
>     > > +        eth_mask.type = match->wc.masks.dl_type;
>     > > +
>     > > +        add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
>     > > +                         &eth_spec, &eth_mask);
>     > > +    } else {
>     > > +        /*
>     > > +         * If user specifies a flow (like UDP flow) without L2 patterns,
>     > > +         * OVS will at least set the dl_type. Normally, it's enough to
>     > > +         * create an eth pattern just with it. Unluckily, some Intel's
>     > > +         * NIC (such as XL710) doesn't support that. Below is a workaround,
>     > > +         * which simply matches any L2 pkts.
>     > > +         */
>     > > +        add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
>     > > +    }
>     > 
>     > This feels a lot like a layer violation - making the core aware
>     > of specific implementation details of lower layers.
>     
>     I agree with you, but unlickily, without it, Intel NIC simply won't work
>     (according to Finn), unless you have mac addr being provided.

I think its reasonable to provide hardware-workarounds. But is it
appropriate to enable it for all hardware and is this the most appropriate
place for a hardware work-around?

>     > >From a functional point of view, is the idea that
>     > a eth_type+5-tuple match is converted to a 5-tuple match?
>     
>     Unluckily, yes.
>     
>     > > +    /* VLAN */
>     > > +    struct rte_flow_item_vlan vlan_spec;
>     > > +    struct rte_flow_item_vlan vlan_mask;
>     > 
>     > Please declare all local variables at the top of the context
>     > (in this case function).
>     > 
>     > ...
>     > 
>     > > +    struct rte_flow_item_udp udp_spec;
>     > > +    struct rte_flow_item_udp udp_mask;
>     > > +    memset(&udp_mask, 0, sizeof(udp_mask));
>     > > +    if ((proto == IPPROTO_UDP) &&
>     > > +        (match->wc.masks.tp_src || match->wc.masks.tp_dst)) {
>     > > +        udp_spec.hdr.src_port = match->flow.tp_src;
>     > > +        udp_spec.hdr.dst_port = match->flow.tp_dst;
>     > > +
>     > > +        udp_mask.hdr.src_port = match->wc.masks.tp_src;
>     > > +        udp_mask.hdr.dst_port = match->wc.masks.tp_dst;
>     > > +
>     > > +        add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_UDP,
>     > > +                         &udp_spec, &udp_mask);
>     > > +
>     > > +        /* proto == UDP and ITEM_TYPE_UDP, thus no need for proto match */
>     > > +        if (ipv4_next_proto_mask) {
>     > > +            *ipv4_next_proto_mask = 0;
>     > 
>     > I think this should be:
>     > 
>     > +            *ipv4_next_proto_mask = NULL;
>     
>     Seems not. ipv4_next_proto_mask is defined as "uint8_t *".

	Sorry, I misread the code.

>     
>     > > +        }
>     > > +    }
>     > > +
>     > > +    struct rte_flow_item_sctp sctp_spec;
>     > > +    struct rte_flow_item_sctp sctp_mask;
>     > > +    memset(&sctp_mask, 0, sizeof(sctp_mask));
>     > > +    if ((proto == IPPROTO_SCTP) &&
>     > 
>     > It seems there are unnecessary () in the line above.
>     
>     yes.
>     
>     > > +/*
>     > > + * Validate for later rte flow offload creation. If any unsupported
>     > > + * flow are specified, return -1.
>     > > + */
>     > > +static int
>     > > +netdev_dpdk_validate_flow(const struct match *match)
>     > > +{
>     > > +    struct match match_zero_wc;
>     > > +
>     > > +    /* Create a wc-zeroed version of flow */
>     > > +    match_init(&match_zero_wc, &match->flow, &match->wc);
>     > > +
>     > > +#define CHECK_NONZERO_BYTES(addr, size) do {    \
>     > 
>     > I think do should appear on a new line.
>     > 
>     > > +    uint8_t *padr = (uint8_t *)(addr);          \
>     > > +    int i;                                      \
>     > > +    for (i = 0; i < (size); i++) {              \
>     > > +        if (padr[i] != 0) {                     \
>     > > +            goto err;                           \
>     > > +        }                                       \
>     > > +    }                                           \
>     > > +} while (0)
>     > > +
>     > > +#define CHECK_NONZERO(var)              do {    \
>     > 
>     > Here too.
>     > 
>     > > +    if ((var) != 0) {                           \
>     > > +        goto err;                               \
>     > > +    }                                           \
>     > > +} while (0)
>     > 
>     > I think its better if macros (and defines) aren't declared
>     > inside of functions. The top of the file seems like
>     > the best place to me. If not above the first function
>     > that uses the macros, presumably this function.
>     
>     Again, I just follow the OVS habit. I saw quite many defines like
>     this in other code pieces. I'm fine to put it outside the function,
>     or even define it as a function though.	

Thanks, my assumption was incorrect here too.

> [Darrell] Pls convert to functions as discussed earlier.

That sounds nice to me.


More information about the dev mailing list