From: ebiederm@xmission•com (Eric W. Biederman)
To: David Ahern <dsa@cumulusnetworks•com>
Cc: netdev@vger•kernel.org, roopa@cumulusnetworks•com, rshearma@brocade•com
Subject: Re: [PATCH net-next 1/4] net: mpls: Convert number of nexthops to u8
Date: Sun, 26 Mar 2017 22:11:51 -0500 [thread overview]
Message-ID: <871stjmn0o.fsf@xmission.com> (raw)
In-Reply-To: <1490461408-9551-2-git-send-email-dsa@cumulusnetworks.com> (David Ahern's message of "Sat, 25 Mar 2017 10:03:25 -0700")
David Ahern <dsa@cumulusnetworks•com> writes:
> Number of nexthops and number of alive nexthops are tracked using an
> unsigned int. A route should never have more than 255 nexthops so
> convert both to u8. Update all references and intermediate variables
> to consistently use u8 as well.
>
> Shrinks the size of mpls_route from 32 bytes to 24 bytes with a 2-byte
> hole before the nexthops.
>
> Also, the ACCESS_ONCE is changed to READ_ONCE per checkpatch message.
I don't like this. Byte writes don't exist on all architectures.
So while I think always writing to rtn_nhn_alive under the
rtn_lock ensures that we don't have wrong values written
it is quite subtle. And I don't know how this will interact with other
fields that you are introducing.
AKA this might be ok, but I expect this formulation of the code
will easily bit-rot and break.
Eric
> Signed-off-by: David Ahern <dsa@cumulusnetworks•com>
> ---
> net/mpls/af_mpls.c | 29 ++++++++++++++++++-----------
> net/mpls/internal.h | 4 ++--
> 2 files changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index cd8be8d5e4ad..d3dc6c43a1d1 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -192,7 +192,7 @@ static u32 mpls_multipath_hash(struct mpls_route *rt, struct sk_buff *skb)
> static struct mpls_nh *mpls_select_multipath(struct mpls_route *rt,
> struct sk_buff *skb)
> {
> - int alive = ACCESS_ONCE(rt->rt_nhn_alive);
> + u8 alive = READ_ONCE(rt->rt_nhn_alive);
> u32 hash = 0;
> int nh_index = 0;
> int n = 0;
> @@ -458,7 +458,7 @@ struct mpls_route_config {
> int rc_mp_len;
> };
>
> -static struct mpls_route *mpls_rt_alloc(int num_nh, u8 max_alen)
> +static struct mpls_route *mpls_rt_alloc(u8 num_nh, u8 max_alen)
> {
> u8 max_alen_aligned = ALIGN(max_alen, VIA_ALEN_ALIGN);
> struct mpls_route *rt;
> @@ -736,11 +736,11 @@ static int mpls_nh_build(struct net *net, struct mpls_route *rt,
> return err;
> }
>
> -static int mpls_count_nexthops(struct rtnexthop *rtnh, int len,
> - u8 cfg_via_alen, u8 *max_via_alen)
> +static u8 mpls_count_nexthops(struct rtnexthop *rtnh, int len,
> + u8 cfg_via_alen, u8 *max_via_alen)
> {
> - int nhs = 0;
> int remaining = len;
> + u8 nhs = 0;
>
> if (!rtnh) {
> *max_via_alen = cfg_via_alen;
> @@ -765,7 +765,14 @@ static int mpls_count_nexthops(struct rtnexthop *rtnh, int len,
> via_alen);
> }
>
> + /* number of nexthops is tracked by a u8.
> + * Check for overflow.
> + */
> + if (nhs == 255)
> + return 0;
> +
> nhs++;
> +
> rtnh = rtnh_next(rtnh, &remaining);
> }
>
> @@ -779,8 +786,8 @@ static int mpls_nh_build_multi(struct mpls_route_config *cfg,
> struct rtnexthop *rtnh = cfg->rc_mp;
> struct nlattr *nla_via, *nla_newdst;
> int remaining = cfg->rc_mp_len;
> - int nhs = 0;
> int err = 0;
> + u8 nhs = 0;
>
> change_nexthops(rt) {
> int attrlen;
> @@ -834,7 +841,7 @@ static int mpls_route_add(struct mpls_route_config *cfg)
> int err = -EINVAL;
> u8 max_via_alen;
> unsigned index;
> - int nhs;
> + u8 nhs;
>
> index = cfg->rc_label;
>
> @@ -1299,8 +1306,8 @@ static void mpls_ifdown(struct net_device *dev, int event)
> struct mpls_route __rcu **platform_label;
> struct net *net = dev_net(dev);
> unsigned int nh_flags = RTNH_F_DEAD | RTNH_F_LINKDOWN;
> - unsigned int alive;
> unsigned index;
> + u8 alive;
>
> platform_label = rtnl_dereference(net->mpls.platform_label);
> for (index = 0; index < net->mpls.platform_labels; index++) {
> @@ -1339,7 +1346,7 @@ static void mpls_ifup(struct net_device *dev, unsigned int nh_flags)
> struct mpls_route __rcu **platform_label;
> struct net *net = dev_net(dev);
> unsigned index;
> - int alive;
> + u8 alive;
>
> platform_label = rtnl_dereference(net->mpls.platform_label);
> for (index = 0; index < net->mpls.platform_labels; index++) {
> @@ -1761,8 +1768,8 @@ static int mpls_dump_route(struct sk_buff *skb, u32 portid, u32 seq, int event,
> } else {
> struct rtnexthop *rtnh;
> struct nlattr *mp;
> - int dead = 0;
> - int linkdown = 0;
> + u8 linkdown = 0;
> + u8 dead = 0;
>
> mp = nla_nest_start(skb, RTA_MULTIPATH);
> if (!mp)
> diff --git a/net/mpls/internal.h b/net/mpls/internal.h
> index 62928d8fabd1..66f388ba2d49 100644
> --- a/net/mpls/internal.h
> +++ b/net/mpls/internal.h
> @@ -123,8 +123,8 @@ struct mpls_route { /* next hop label forwarding entry */
> u8 rt_payload_type;
> u8 rt_max_alen;
> u8 rt_ttl_propagate;
> - unsigned int rt_nhn;
> - unsigned int rt_nhn_alive;
> + u8 rt_nhn;
> + u8 rt_nhn_alive;
> struct mpls_nh rt_nh[0];
> };
next prev parent reply other threads:[~2017-03-27 3:17 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-25 17:03 [PATCH net-next 0/4] net: mpls: Allow users to configure more labels per route David Ahern
2017-03-25 17:03 ` [PATCH net-next 1/4] net: mpls: Convert number of nexthops to u8 David Ahern
2017-03-27 3:11 ` Eric W. Biederman [this message]
2017-03-27 14:43 ` David Ahern
2017-03-27 22:54 ` Eric W. Biederman
2017-03-28 15:25 ` David Ahern
2017-03-28 18:39 ` Eric W. Biederman
2017-03-25 17:03 ` [PATCH net-next 2/4] net: mpls: change mpls_route layout David Ahern
2017-03-28 0:04 ` Eric W. Biederman
2017-03-25 17:03 ` [PATCH net-next 3/4] net: mpls: bump maximum number of labels David Ahern
2017-03-25 17:03 ` [PATCH net-next 4/4] net: mpls: Increase max number of labels for lwt encap David Ahern
2017-03-25 19:15 ` [PATCH net-next 0/4] net: mpls: Allow users to configure more labels per route Eric W. Biederman
2017-03-27 10:39 ` Robert Shearman
2017-03-27 14:21 ` David Ahern
2017-03-28 3:08 ` Eric W. Biederman
2017-03-28 9:52 ` Robert Shearman
2017-03-28 14:39 ` David Ahern
2017-03-29 21:20 ` David Ahern
2017-03-27 22:52 ` David Miller
2017-03-28 9:59 ` Robert Shearman
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=871stjmn0o.fsf@xmission.com \
--to=ebiederm@xmission$(echo .)com \
--cc=dsa@cumulusnetworks$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=roopa@cumulusnetworks$(echo .)com \
--cc=rshearma@brocade$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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