Re: [RFC PATCH] lib: Introduce generic __cmpxchg_u64() and use it where needed

[Trond Myklebust]

On Wed, 2018-10-31 at 18:18 -0700, Guenter Roeck wrote:
> On 10/31/18 4:32 PM, Paul Burton wrote:
> > (Copying SunRPC & net maintainers.)
> > 
> > Hi Guenter,
> > 
> > On Wed, Oct 31, 2018 at 03:02:53PM -0700, Guenter Roeck wrote:
> > > The alternatives I can see are
> > > - Do not use cmpxchg64() outside architecture code (ie drop its
> > > use from
> > >    the offending driver, and keep doing the same whenever the
> > > problem comes
> > >    up again).
> > > or
> > > - Introduce something like ARCH_HAS_CMPXCHG64 and use it to
> > > determine
> > >    if cmpxchg64 is supported or not.
> > > 
> > > Any preference ?
> > 
> > My preference would be option 1 - avoiding cmpxchg64() where
> > possible in
> > generic code. I wouldn't be opposed to the Kconfig option if there
> > are
> > cases where cmpxchg64() can really help performance though.
> > 
> > The last time I'm aware of this coming up the affected driver was
> > modified to avoid cmpxchg64() [1].
> > 
> > In this particular case I have no idea why
> > net/sunrpc/auth_gss/gss_krb5_seal.c is using cmpxchg64() at all.
> > It's
> > essentially reinventing atomic64_fetch_inc() which is already
> > provided
> > everywhere via CONFIG_GENERIC_ATOMIC64 & the spinlock approach. At
> > least
> > for atomic64_* functions the assumption that all access will be
> > performed using those same functions seems somewhat reasonable.
> > 
> > So how does the below look? Trond?
> > 
> 
> For my part I agree that this would be a much better solution. The
> argument
> that it is not always absolutely guaranteed that atomics don't wrap
> doesn't
> really hold for me because it looks like they all do. On top of that,
> there
> is an explicit atomic_dec_if_positive() and
> atomic_fetch_add_unless(),
> which to me strongly suggests that they _are_ supposed to wrap.
> Given the cost of adding a comparison to each atomic operation to
> prevent it from wrapping, anything else would not really make sense
> to me.

That's a hypothesis, not a proven fact. There are architectures out
there that do not wrap signed integers, hence my question.

> So ... please consider my patch abandoned. Thanks for looking into
> this!
> 
> Guenter
> 
> > Thanks,
> >      Paul
> > 
> > [1] https://patchwork.ozlabs.org/cover/891284/
> > 
> > ---
> > diff --git a/include/linux/sunrpc/gss_krb5.h
> > b/include/linux/sunrpc/gss_krb5.h
> > index 131424cefc6a..02c0412e368c 100644
> > --- a/include/linux/sunrpc/gss_krb5.h
> > +++ b/include/linux/sunrpc/gss_krb5.h
> > @@ -107,8 +107,8 @@ struct krb5_ctx {
> >   	u8			Ksess[GSS_KRB5_MAX_KEYLEN]; /* session key
> > */
> >   	u8			cksum[GSS_KRB5_MAX_KEYLEN];
> >   	s32			endtime;
> > -	u32			seq_send;
> > -	u64			seq_send64;
> > +	atomic_t		seq_send;
> > +	atomic64_t		seq_send64;
> >   	struct xdr_netobj	mech_used;
> >   	u8			initiator_sign[GSS_KRB5_MAX_KEYLEN];
> >   	u8			acceptor_sign[GSS_KRB5_MAX_KEYLEN];
> > @@ -118,9 +118,6 @@ struct krb5_ctx {
> >   	u8			acceptor_integ[GSS_KRB5_MAX_KEYLEN];
> >   };
> >   
> > -extern u32 gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx);
> > -extern u64 gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx);
> > -
> >   /* The length of the Kerberos GSS token header */
> >   #define GSS_KRB5_TOK_HDR_LEN	(16)
> >   
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_mech.c
> > b/net/sunrpc/auth_gss/gss_krb5_mech.c
> > index 7f0424dfa8f6..eab71fc7af3e 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_mech.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_mech.c
> > @@ -274,6 +274,7 @@ get_key(const void *p, const void *end,
> >   static int
> >   gss_import_v1_context(const void *p, const void *end, struct
> > krb5_ctx *ctx)
> >   {
> > +	u32 seq_send;
> >   	int tmp;
> >   
> >   	p = simple_get_bytes(p, end, &ctx->initiate, sizeof(ctx-
> > >initiate));
> > @@ -315,9 +316,10 @@ gss_import_v1_context(const void *p, const
> > void *end, struct krb5_ctx *ctx)
> >   	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx-
> > >endtime));
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > -	p = simple_get_bytes(p, end, &ctx->seq_send, sizeof(ctx-
> > >seq_send));
> > +	p = simple_get_bytes(p, end, &seq_send, sizeof(seq_send));
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > +	atomic_set(&ctx->seq_send, seq_send);
> >   	p = simple_get_netobj(p, end, &ctx->mech_used);
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > @@ -607,6 +609,7 @@ static int
> >   gss_import_v2_context(const void *p, const void *end, struct
> > krb5_ctx *ctx,
> >   		gfp_t gfp_mask)
> >   {
> > +	u64 seq_send64;
> >   	int keylen;
> >   
> >   	p = simple_get_bytes(p, end, &ctx->flags, sizeof(ctx->flags));
> > @@ -617,14 +620,15 @@ gss_import_v2_context(const void *p, const
> > void *end, struct krb5_ctx *ctx,
> >   	p = simple_get_bytes(p, end, &ctx->endtime, sizeof(ctx-
> > >endtime));
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > -	p = simple_get_bytes(p, end, &ctx->seq_send64, sizeof(ctx-
> > >seq_send64));
> > +	p = simple_get_bytes(p, end, &seq_send64, sizeof(seq_send64));
> >   	if (IS_ERR(p))
> >   		goto out_err;
> > +	atomic64_set(&ctx->seq_send64, seq_send64);
> >   	/* set seq_send for use by "older" enctypes */
> > -	ctx->seq_send = ctx->seq_send64;
> > -	if (ctx->seq_send64 != ctx->seq_send) {
> > -		dprintk("%s: seq_send64 %lx, seq_send %x overflow?\n",
> > __func__,
> > -			(unsigned long)ctx->seq_send64, ctx->seq_send);
> > +	atomic_set(&ctx->seq_send, seq_send64);
> > +	if (seq_send64 != atomic_read(&ctx->seq_send)) {
> > +		dprintk("%s: seq_send64 %llx, seq_send %x overflow?\n",
> > __func__,
> > +			seq_send64, atomic_read(&ctx->seq_send));
> >   		p = ERR_PTR(-EINVAL);
> >   		goto out_err;
> >   	}
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_seal.c
> > b/net/sunrpc/auth_gss/gss_krb5_seal.c
> > index b4adeb06660b..48fe4a591b54 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_seal.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_seal.c
> > @@ -123,30 +123,6 @@ setup_token_v2(struct krb5_ctx *ctx, struct
> > xdr_netobj *token)
> >   	return krb5_hdr;
> >   }
> >   
> > -u32
> > -gss_seq_send_fetch_and_inc(struct krb5_ctx *ctx)
> > -{
> > -	u32 old, seq_send = READ_ONCE(ctx->seq_send);
> > -
> > -	do {
> > -		old = seq_send;
> > -		seq_send = cmpxchg(&ctx->seq_send, old, old + 1);
> > -	} while (old != seq_send);
> > -	return seq_send;
> > -}
> > -
> > -u64
> > -gss_seq_send64_fetch_and_inc(struct krb5_ctx *ctx)
> > -{
> > -	u64 old, seq_send = READ_ONCE(ctx->seq_send);
> > -
> > -	do {
> > -		old = seq_send;
> > -		seq_send = cmpxchg64(&ctx->seq_send64, old, old + 1);
> > -	} while (old != seq_send);
> > -	return seq_send;
> > -}
> > -
> >   static u32
> >   gss_get_mic_v1(struct krb5_ctx *ctx, struct xdr_buf *text,
> >   		struct xdr_netobj *token)
> > @@ -177,7 +153,7 @@ gss_get_mic_v1(struct krb5_ctx *ctx, struct
> > xdr_buf *text,
> >   
> >   	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data,
> > md5cksum.len);
> >   
> > -	seq_send = gss_seq_send_fetch_and_inc(ctx);
> > +	seq_send = atomic_fetch_inc(&ctx->seq_send);
> >   
> >   	if (krb5_make_seq_num(ctx, ctx->seq, ctx->initiate ? 0 : 0xff,
> >   			      seq_send, ptr + GSS_KRB5_TOK_HDR_LEN, ptr
> > + 8))
> > @@ -205,7 +181,7 @@ gss_get_mic_v2(struct krb5_ctx *ctx, struct
> > xdr_buf *text,
> >   
> >   	/* Set up the sequence number. Now 64-bits in clear
> >   	 * text and w/o direction indicator */
> > -	seq_send_be64 = cpu_to_be64(gss_seq_send64_fetch_and_inc(ctx));
> > +	seq_send_be64 = cpu_to_be64(atomic64_fetch_inc(&ctx-
> > >seq_send64));
> >   	memcpy(krb5_hdr + 8, (char *) &seq_send_be64, 8);
> >   
> >   	if (ctx->initiate) {
> > diff --git a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > index 962fa84e6db1..5cdde6cb703a 100644
> > --- a/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > +++ b/net/sunrpc/auth_gss/gss_krb5_wrap.c
> > @@ -228,7 +228,7 @@ gss_wrap_kerberos_v1(struct krb5_ctx *kctx, int
> > offset,
> >   
> >   	memcpy(ptr + GSS_KRB5_TOK_HDR_LEN, md5cksum.data,
> > md5cksum.len);
> >   
> > -	seq_send = gss_seq_send_fetch_and_inc(kctx);
> > +	seq_send = atomic_fetch_inc(&kctx->seq_send);
> >   
> >   	/* XXX would probably be more efficient to compute checksum
> >   	 * and encrypt at the same time: */
> > @@ -475,7 +475,7 @@ gss_wrap_kerberos_v2(struct krb5_ctx *kctx, u32
> > offset,
> >   	*be16ptr++ = 0;
> >   
> >   	be64ptr = (__be64 *)be16ptr;
> > -	*be64ptr = cpu_to_be64(gss_seq_send64_fetch_and_inc(kctx));
> > +	*be64ptr = cpu_to_be64(atomic64_fetch_inc(&kctx->seq_send64));
> >   
> >   	err = (*kctx->gk5e->encrypt_v2)(kctx, offset, buf, pages);
> >   	if (err)
> > 
-- 
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space