On 9/10/17, 11:11 PM, "ovs-dev-boun...@openvswitch.org on behalf of Yuanhan
Liu" <ovs-dev-boun...@openvswitch.org on behalf of y...@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 <f...@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.
> > + 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.
> > + memset(ð_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(ð_spec.dst, &match->flow.dl_dst,
sizeof(eth_spec.dst));
> > + rte_memcpy(ð_spec.src, &match->flow.dl_src,
sizeof(eth_spec.src));
> > + eth_spec.type = match->flow.dl_type;
> > +
> > + rte_memcpy(ð_mask.dst, &match->wc.masks.dl_dst,
> > + sizeof(eth_mask.dst));
> > + rte_memcpy(ð_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,
> > + ð_spec, ð_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.
> >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 *".
> > + }
> > + }
> > +
> > + 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.
[Darrell] Pls convert to functions as discussed earlier.
--yliu
_______________________________________________
dev mailing list
d...@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=mVDsTNxOCpGJWdPt_BIHsPGlF-FSjSCIMFXRntq4ww4&s=QIQvaFzRDNUYTMXxRET9DIRtkVfULAPQYgNcuo8620o&e=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
