Re: [PATCH bpf-next 1/3] net: bonding: Add XDP support to the bonding driver

[Jussi Maki <joamaki@gmail.com>]

On Thu, Jun 10, 2021 at 1:29 AM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>         The design adds logic around a bpf_bond_redirect_enabled_key
> static key in the BPF core functions dev_map_enqueue_multi,
> dev_map_redirect_multi and bpf_prog_run_xdp.  Is this something that is
> correctly implemented as a special case just for bonding (i.e., it will
> never ever have to be extended), or is it possible that other
> upper/lower type software devices will have similar XDP functionality
> added in the future, e.g., bridge, VLAN, etc?

Good point. For example the "team" driver would basically need pretty
much the same implementation. For that just using non-bond naming
would be enough. I don't think there's much of a cost for doing a more
generic mechanism, e.g. xdp "upper intercept" hook in netdev_ops, so
I'll try that out. At the very least I'll change the naming.

...

> >@@ -3543,26 +3614,30 @@ static u32 bond_vlan_srcmac_hash(struct sk_buff *skb)
> > }
> >
> > /* Extract the appropriate headers based on bond's xmit policy */
> >-static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
> >+static bool bond_flow_dissect(struct bonding *bond,
> >+                            struct sk_buff *skb,
> >+                            const void *data,
> >+                            __be16 l2_proto,
> >+                            int nhoff,
> >+                            int hlen,
> >                             struct flow_keys *fk)
>
>         Please compact the argument list down to fewer lines, in
> conformance with usual coding practice in the kernel.  The above style
> of formatting occurs multiple times in this patch, both in function
> declarations and function calls.

Thanks will do.

...

> >-/**
> >- * bond_xmit_hash - generate a hash value based on the xmit policy
> >- * @bond: bonding device
> >- * @skb: buffer to use for headers
> >- *
> >- * This function will extract the necessary headers from the skb buffer and use
> >- * them to generate a hash based on the xmit_policy set in the bonding device
> >+/* Generate hash based on xmit policy. If @skb is given it is used to linearize
> >+ * the data as required, but this function can be used without it.
>
>         Please don't remove kernel-doc formatting; add your new
> parameters to the documentation.

The comment and the function declaration were untouched (see further
below in patch).  I only introduced the common helper __bond_xmit_hash
used from bond_xmit_hash and bond_xmit_hash_xdp. Unfortunately the
generated diff was a bit confusing. I'll try and generate cleaner
diffs in the future.

> >  */
> >-u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> >+static u32 __bond_xmit_hash(struct bonding *bond,
> >+                          struct sk_buff *skb,
> >+                          const void *data,
> >+                          __be16 l2_proto,
> >+                          int mhoff,
> >+                          int nhoff,
> >+                          int hlen)
> > {
> >       struct flow_keys flow;
> >       u32 hash;
> >
> >-      if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> >-          skb->l4_hash)
> >-              return skb->hash;
> >-
> >       if (bond->params.xmit_policy == BOND_XMIT_POLICY_VLAN_SRCMAC)
> >-              return bond_vlan_srcmac_hash(skb);
> >+              return bond_vlan_srcmac_hash(skb, data, mhoff, hlen);
> >
> >       if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
> >-          !bond_flow_dissect(bond, skb, &flow))
> >-              return bond_eth_hash(skb);
> >+          !bond_flow_dissect(bond, skb, data, l2_proto, nhoff, hlen, &flow))
> >+              return bond_eth_hash(skb, data, mhoff, hlen);
> >
> >       if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER23 ||
> >           bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23) {
> >-              hash = bond_eth_hash(skb);
> >+              hash = bond_eth_hash(skb, data, mhoff, hlen);
> >       } else {
> >               if (flow.icmp.id)
> >                       memcpy(&hash, &flow.icmp, sizeof(hash));
> >@@ -3638,6 +3708,48 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> >       return bond_ip_hash(hash, &flow);
> > }
> >
> >+/**
> >+ * bond_xmit_hash_skb - generate a hash value based on the xmit policy
> >+ * @bond: bonding device
> >+ * @skb: buffer to use for headers
> >+ *
> >+ * This function will extract the necessary headers from the skb buffer and use
> >+ * them to generate a hash based on the xmit_policy set in the bonding device
> >+ */
> >+u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> >+{
> >+      if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> >+          skb->l4_hash)
> >+              return skb->hash;
> >+
> >+      return __bond_xmit_hash(bond, skb, skb->head, skb->protocol,
> >+                              skb->mac_header,
> >+                              skb->network_header,
> >+                              skb_headlen(skb));
> >+}
...