From: David Miller <davem@davemloft.net>
To: paul.durrant@citrix.com
Cc: netdev@vger.kernel.org, j.vosburgh@gmail.com, vfalico@gmail.com,
gospo@cumulusnetworks.com, tom@herbertland.com,
eric.dumazet@gmail.com
Subject: Re: [PATCH net-next] store complete hash type information in socket buffer...
Date: Wed, 17 Feb 2016 15:44:32 -0500 (EST) [thread overview]
Message-ID: <20160217.154432.788293606088459412.davem@davemloft.net> (raw)
In-Reply-To: <1455525128-27795-1-git-send-email-paul.durrant@citrix.com>
From: Paul Durrant <paul.durrant@citrix.com>
Date: Mon, 15 Feb 2016 08:32:08 +0000
> ...rather than a boolean merely indicating a canonical L4 hash.
>
> skb_set_hash() takes a hash type (from enum pkt_hash_types) as an
> argument but information is lost since only a single bit in the skb
> stores whether that hash type is PKT_HASH_TYPE_L4 or not. By using
> two bits it's possible to store the complete hash type information.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Tom and/or Eric, please have a look at this.
> ---
> drivers/net/bonding/bond_main.c | 2 +-
> include/linux/skbuff.h | 53 ++++++++++++++++++++++++++++-------------
> include/net/flow_dissector.h | 5 ++++
> include/net/sock.h | 2 +-
> include/trace/events/net.h | 2 +-
> net/core/flow_dissector.c | 27 ++++++++++++++++-----
> 6 files changed, 65 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 45bdd87..ad0ee00 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3173,7 +3173,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> u32 hash;
>
> if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
> - skb->l4_hash)
> + skb_has_l4_hash(skb))
> return skb->hash;
>
> if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 3920675..b991ca2 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -595,8 +595,7 @@ static inline bool skb_mstamp_after(const struct skb_mstamp *t1,
> * @xmit_more: More SKBs are pending for this queue
> * @ndisc_nodetype: router type (from link layer)
> * @ooo_okay: allow the mapping of a socket to a queue to be changed
> - * @l4_hash: indicate hash is a canonical 4-tuple hash over transport
> - * ports.
> + * @hash_type: indicates type of hash (see enum pkt_hash_types below)
> * @sw_hash: indicates hash was computed in software stack
> * @wifi_acked_valid: wifi_acked was set
> * @wifi_acked: whether frame was acked on wifi or not
> @@ -701,10 +700,10 @@ struct sk_buff {
> __u8 nf_trace:1;
> __u8 ip_summed:2;
> __u8 ooo_okay:1;
> - __u8 l4_hash:1;
> __u8 sw_hash:1;
> __u8 wifi_acked_valid:1;
> __u8 wifi_acked:1;
> + /* 1 bit hole */
>
> __u8 no_fcs:1;
> /* Indicates the inner headers are valid in the skbuff. */
> @@ -721,7 +720,8 @@ struct sk_buff {
> __u8 ipvs_property:1;
> __u8 inner_protocol_type:1;
> __u8 remcsum_offload:1;
> - /* 3 or 5 bit hole */
> + __u8 hash_type:2;
> + /* 1 or 3 bit hole */
>
> #ifdef CONFIG_NET_SCHED
> __u16 tc_index; /* traffic control index */
> @@ -1030,19 +1030,35 @@ static inline void skb_clear_hash(struct sk_buff *skb)
> {
> skb->hash = 0;
> skb->sw_hash = 0;
> - skb->l4_hash = 0;
> + skb->hash_type = 0;
> +}
> +
> +static inline enum pkt_hash_types skb_hash_type(struct sk_buff *skb)
> +{
> + return skb->hash_type;
> +}
> +
> +static inline bool skb_has_l4_hash(struct sk_buff *skb)
> +{
> + return skb_hash_type(skb) == PKT_HASH_TYPE_L4;
> +}
> +
> +static inline bool skb_has_sw_hash(struct sk_buff *skb)
> +{
> + return !!skb->sw_hash;
> }
>
> static inline void skb_clear_hash_if_not_l4(struct sk_buff *skb)
> {
> - if (!skb->l4_hash)
> + if (!skb_has_l4_hash(skb))
> skb_clear_hash(skb);
> }
>
> static inline void
> -__skb_set_hash(struct sk_buff *skb, __u32 hash, bool is_sw, bool is_l4)
> +__skb_set_hash(struct sk_buff *skb, __u32 hash, bool is_sw,
> + enum pkt_hash_types type)
> {
> - skb->l4_hash = is_l4;
> + skb->hash_type = type;
> skb->sw_hash = is_sw;
> skb->hash = hash;
> }
> @@ -1051,13 +1067,13 @@ static inline void
> skb_set_hash(struct sk_buff *skb, __u32 hash, enum pkt_hash_types type)
> {
> /* Used by drivers to set hash from HW */
> - __skb_set_hash(skb, hash, false, type == PKT_HASH_TYPE_L4);
> + __skb_set_hash(skb, hash, false, type);
> }
>
> static inline void
> -__skb_set_sw_hash(struct sk_buff *skb, __u32 hash, bool is_l4)
> +__skb_set_sw_hash(struct sk_buff *skb, __u32 hash, enum pkt_hash_types type)
> {
> - __skb_set_hash(skb, hash, true, is_l4);
> + __skb_set_hash(skb, hash, true, type);
> }
>
> void __skb_get_hash(struct sk_buff *skb);
> @@ -1110,9 +1126,10 @@ static inline bool skb_flow_dissect_flow_keys_buf(struct flow_keys *flow,
> data, proto, nhoff, hlen, flags);
> }
>
> +
> static inline __u32 skb_get_hash(struct sk_buff *skb)
> {
> - if (!skb->l4_hash && !skb->sw_hash)
> + if (!skb_has_l4_hash(skb) && !skb_has_sw_hash(skb))
> __skb_get_hash(skb);
>
> return skb->hash;
> @@ -1122,11 +1139,12 @@ __u32 __skb_get_hash_flowi6(struct sk_buff *skb, const struct flowi6 *fl6);
>
> static inline __u32 skb_get_hash_flowi6(struct sk_buff *skb, const struct flowi6 *fl6)
> {
> - if (!skb->l4_hash && !skb->sw_hash) {
> + if (!skb_has_l4_hash(skb) && !skb_has_sw_hash(skb)) {
> struct flow_keys keys;
> __u32 hash = __get_hash_from_flowi6(fl6, &keys);
>
> - __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys));
> + __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys) ?
> + PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
> }
>
> return skb->hash;
> @@ -1136,11 +1154,12 @@ __u32 __skb_get_hash_flowi4(struct sk_buff *skb, const struct flowi4 *fl);
>
> static inline __u32 skb_get_hash_flowi4(struct sk_buff *skb, const struct flowi4 *fl4)
> {
> - if (!skb->l4_hash && !skb->sw_hash) {
> + if (!skb_has_l4_hash(skb) && !skb_has_sw_hash(skb)) {
> struct flow_keys keys;
> __u32 hash = __get_hash_from_flowi4(fl4, &keys);
>
> - __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys));
> + __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys) ?
> + PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
> }
>
> return skb->hash;
> @@ -1157,7 +1176,7 @@ static inline void skb_copy_hash(struct sk_buff *to, const struct sk_buff *from)
> {
> to->hash = from->hash;
> to->sw_hash = from->sw_hash;
> - to->l4_hash = from->l4_hash;
> + to->hash_type = from->hash_type;
> };
>
> static inline void skb_sender_cpu_clear(struct sk_buff *skb)
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 8c8548c..418b8c5 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -182,6 +182,11 @@ static inline bool flow_keys_have_l4(struct flow_keys *keys)
> return (keys->ports.ports || keys->tags.flow_label);
> }
>
> +static inline bool flow_keys_have_l3(struct flow_keys *keys)
> +{
> + return !!keys->control.addr_type;
> +}
> +
> u32 flow_hash_from_keys(struct flow_keys *keys);
>
> #endif
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 255d3e0..21b8cdc 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1828,7 +1828,7 @@ static inline void sock_poll_wait(struct file *filp,
> static inline void skb_set_hash_from_sk(struct sk_buff *skb, struct sock *sk)
> {
> if (sk->sk_txhash) {
> - skb->l4_hash = 1;
> + skb->hash_type = PKT_HASH_TYPE_L4;
> skb->hash = sk->sk_txhash;
> }
> }
> diff --git a/include/trace/events/net.h b/include/trace/events/net.h
> index 49cc7c3..25e7979 100644
> --- a/include/trace/events/net.h
> +++ b/include/trace/events/net.h
> @@ -180,7 +180,7 @@ DECLARE_EVENT_CLASS(net_dev_rx_verbose_template,
> __entry->protocol = ntohs(skb->protocol);
> __entry->ip_summed = skb->ip_summed;
> __entry->hash = skb->hash;
> - __entry->l4_hash = skb->l4_hash;
> + __entry->l4_hash = skb->hash_type == PKT_HASH_TYPE_L4;
> __entry->len = skb->len;
> __entry->data_len = skb->data_len;
> __entry->truesize = skb->truesize;
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index d79699c..956208b 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -658,17 +658,30 @@ EXPORT_SYMBOL(make_flow_keys_digest);
> *
> * This function calculates a flow hash based on src/dst addresses
> * and src/dst port numbers. Sets hash in skb to non-zero hash value
> - * on success, zero indicates no valid hash. Also, sets l4_hash in skb
> - * if hash is a canonical 4-tuple hash over transport ports.
> + * on success, zero indicates no valid hash in which case the hash type
> + * is set to NONE. If the hash is a canonical 4-tuple hash over transport
> + * ports then type is set to L4. If the hash did not include transport
> + * then type is set to L3, otherwise it is assumed to be L2 only.
> */
> void __skb_get_hash(struct sk_buff *skb)
> {
> struct flow_keys keys;
> + u32 hash;
> + enum pkt_hash_types type;
>
> __flow_hash_secret_init();
>
> - __skb_set_sw_hash(skb, ___skb_get_hash(skb, &keys, hashrnd),
> - flow_keys_have_l4(&keys));
> + hash = ___skb_get_hash(skb, &keys, hashrnd);
> + if (hash == 0)
> + type = PKT_HASH_TYPE_NONE;
> + else if (flow_keys_have_l4(&keys))
> + type = PKT_HASH_TYPE_L4;
> + else if (flow_keys_have_l3(&keys))
> + type = PKT_HASH_TYPE_L3;
> + else
> + type = PKT_HASH_TYPE_L2;
> +
> + __skb_set_sw_hash(skb, hash, type);
> }
> EXPORT_SYMBOL(__skb_get_hash);
>
> @@ -698,7 +711,8 @@ __u32 __skb_get_hash_flowi6(struct sk_buff *skb, const struct flowi6 *fl6)
> keys.basic.ip_proto = fl6->flowi6_proto;
>
> __skb_set_sw_hash(skb, flow_hash_from_keys(&keys),
> - flow_keys_have_l4(&keys));
> + flow_keys_have_l4(&keys) ?
> + PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
>
> return skb->hash;
> }
> @@ -719,7 +733,8 @@ __u32 __skb_get_hash_flowi4(struct sk_buff *skb, const struct flowi4 *fl4)
> keys.basic.ip_proto = fl4->flowi4_proto;
>
> __skb_set_sw_hash(skb, flow_hash_from_keys(&keys),
> - flow_keys_have_l4(&keys));
> + flow_keys_have_l4(&keys) ?
> + PKT_HASH_TYPE_L4 : PKT_HASH_TYPE_L3);
>
> return skb->hash;
> }
> --
> 2.1.4
>
next prev parent reply other threads:[~2016-02-17 20:44 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-15 8:32 [PATCH net-next] store complete hash type information in socket buffer Paul Durrant
2016-02-17 9:01 ` Paul Durrant
2016-02-17 20:44 ` David Miller [this message]
2016-02-17 21:30 ` Eric Dumazet
2016-02-19 2:22 ` Tom Herbert
2016-02-19 8:55 ` Paul Durrant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160217.154432.788293606088459412.davem@davemloft.net \
--to=davem@davemloft.net \
--cc=eric.dumazet@gmail.com \
--cc=gospo@cumulusnetworks.com \
--cc=j.vosburgh@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=paul.durrant@citrix.com \
--cc=tom@herbertland.com \
--cc=vfalico@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).