On Thu, Jun 10, 2021 at 1:29 AM Jay Vosburgh <jay.vosburgh@xxxxxxxxxxxxx> 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));
> >+}
...
