From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Subject: Re: [PATCH v6 1/2] eal/ticketlock: ticket based to
 improve	fairness
Date: Fri, 15 Mar 2019 12:55:59 +0000
Message-ID: <2601191342CEEE43887BDE71AB977258013655BF89@irsmsx105.ger.corp.intel.com>
References: <1547802943-18711-1-git-send-email-joyce.kong@arm.com>
 <1552632988-80787-2-git-send-email-joyce.kong@arm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Cc: "nd@arm.com" <nd@arm.com>, "stephen@networkplumber.org"
 <stephen@networkplumber.org>, "jerin.jacob@caviumnetworks.com"
 <jerin.jacob@caviumnetworks.com>, "thomas@monjalon.net"
 <thomas@monjalon.net>, "honnappa.nagarahalli@arm.com"
 <honnappa.nagarahalli@arm.com>, "gavin.hu@arm.com" <gavin.hu@arm.com>
To: Joyce Kong <joyce.kong@arm.com>, "dev@dpdk.org" <dev@dpdk.org>
Return-path: <dev-bounces@dpdk.org>
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
 by dpdk.org (Postfix) with ESMTP id 032B92C24
 for <dev@dpdk.org>; Fri, 15 Mar 2019 13:56:03 +0100 (CET)
In-Reply-To: <1552632988-80787-2-git-send-email-joyce.kong@arm.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi,

> diff --git a/lib/librte_eal/common/include/generic/rte_ticketlock.h b/lib=
/librte_eal/common/include/generic/rte_ticketlock.h
> new file mode 100644
> index 0000000..d63aaaa
> --- /dev/null
> +++ b/lib/librte_eal/common/include/generic/rte_ticketlock.h
> @@ -0,0 +1,308 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2019 Arm Limited
> + */
> +
> +#ifndef _RTE_TICKETLOCK_H_
> +#define _RTE_TICKETLOCK_H_
> +
> +/**
> + * @file
> + *
> + * RTE ticket locks
> + *
> + * This file defines an API for ticket locks, which give each waiting
> + * thread a ticket and take the lock one by one, first come, first
> + * serviced.
> + *
> + * All locks must be initialised before use, and only initialised once.
> + *
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <rte_common.h>
> +#include <rte_lcore.h>
> +#include <rte_pause.h>
> +
> +/**
> + * The rte_ticketlock_t type.
> + */
> +typedef struct {
> +	uint16_t current;
> +	uint16_t next;
> +} rte_ticketlock_t;
> +
> +/**
> + * A static ticketlock initializer.
> + */
> +#define RTE_TICKETLOCK_INITIALIZER { 0 }
> +
> +/**
> + * Initialize the ticketlock to an unlocked state.
> + *
> + * @param tl
> + *   A pointer to the ticketlock.
> + */
> +static inline __rte_experimental void
> +rte_ticketlock_init(rte_ticketlock_t *tl)
> +{
> +	__atomic_store_n(&tl->current, 0, __ATOMIC_RELAXED);
> +	__atomic_store_n(&tl->next, 0, __ATOMIC_RELAXED);
> +}
> +
> +/**
> + * Take the ticketlock.
> + *
> + * @param tl
> + *   A pointer to the ticketlock.
> + */
> +static inline __rte_experimental void
> +rte_ticketlock_lock(rte_ticketlock_t *tl)
> +{
> +	uint16_t me =3D __atomic_fetch_add(&tl->next, 1, __ATOMIC_RELAXED);
> +	while (__atomic_load_n(&tl->current, __ATOMIC_ACQUIRE) !=3D me)
> +		rte_pause();
> +}
> +
> +/**
> + * Release the ticketlock.
> + *
> + * @param tl
> + *   A pointer to the ticketlock.
> + */
> +static inline __rte_experimental void
> +rte_ticketlock_unlock(rte_ticketlock_t *tl)
> +{
> +	uint16_t i =3D __atomic_load_n(&tl->current, __ATOMIC_RELAXED);
> +	__atomic_store_n(&tl->current, i+1, __ATOMIC_RELEASE);
> +}
> +
> +/**
> + * Try to take the lock.
> + *
> + * @param tl
> + *   A pointer to the ticketlock.
> + * @return
> + *   1 if the lock is successfully taken; 0 otherwise.
> + */
> +static inline __rte_experimental int
> +rte_ticketlock_trylock(rte_ticketlock_t *tl)
> +{
> +	uint16_t next =3D __atomic_load_n(&tl->next, __ATOMIC_RELAXED);
> +	uint16_t cur =3D __atomic_load_n(&tl->current, __ATOMIC_RELAXED);
> +	if (next =3D=3D cur) {

Probably a na=EFve one:
Suppose next=3D=3Dcur=3D=3D1 here, then this thread will experience really =
long context switch,
so next time it continues its execution tl->next value will wrap-up and wil=
l be 1 again, and tl->current=3D=3D0 (lock held).
I suppose this function will set tl->next=3D2 and will return a success?
Wouldn't be better here and in _is_locked_ to do load/store for next/curren=
t values in one go
(using 32bit op)?=20
Konstantin

> +		if (__atomic_compare_exchange_n(&tl->next, &next, next+1,
> +		    0, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED))
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * Test if the lock is taken.
> + *
> + * @param tl
> + *   A pointer to the ticketlock.
> + * @return
> + *   1 if the lock icurrently taken; 0 otherwise.
> + */
> +static inline __rte_experimental int
> +rte_ticketlock_is_locked(rte_ticketlock_t *tl)
> +{
> +	return (__atomic_load_n(&tl->current, __ATOMIC_ACQUIRE) !=3D
> +		__atomic_load_n(&tl->next, __ATOMIC_ACQUIRE));
> +}
> +