Mailing List Archive

[PATCH v2 05/13] tools/xenstore: make some write limit functions static
Some wrl_*() functions are only used in xenstored_domain.c, so make
them static. In order to avoid the need of forward declarations, move
the whole function block to the start of the file.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/xenstore/xenstored_domain.c | 456 +++++++++++++++---------------
tools/xenstore/xenstored_domain.h | 3 -
2 files changed, 228 insertions(+), 231 deletions(-)

diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index be9175a461..0a8353dad5 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -159,6 +159,234 @@ struct changed_domain

static struct hashtable *domhash;

+static wrl_creditt wrl_config_writecost = WRL_FACTOR;
+static wrl_creditt wrl_config_rate = WRL_RATE * WRL_FACTOR;
+static wrl_creditt wrl_config_dburst = WRL_DBURST * WRL_FACTOR;
+static wrl_creditt wrl_config_gburst = WRL_GBURST * WRL_FACTOR;
+static wrl_creditt wrl_config_newdoms_dburst =
+ WRL_DBURST * WRL_NEWDOMS * WRL_FACTOR;
+
+long wrl_ntransactions;
+
+static long wrl_ndomains;
+static wrl_creditt wrl_reserve; /* [-wrl_config_newdoms_dburst, +_gburst ] */
+static time_t wrl_log_last_warning; /* 0: no previous warning */
+
+#define trace_wrl(...) \
+do { \
+ if (trace_flags & TRACE_WRL) \
+ trace("wrl: " __VA_ARGS__); \
+} while (0)
+
+void wrl_gettime_now(struct wrl_timestampt *now_wt)
+{
+ struct timespec now_ts;
+ int r;
+
+ r = clock_gettime(CLOCK_MONOTONIC, &now_ts);
+ if (r)
+ barf_perror("Could not find time (clock_gettime failed)");
+
+ now_wt->sec = now_ts.tv_sec;
+ now_wt->msec = now_ts.tv_nsec / 1000000;
+}
+
+static void wrl_xfer_credit(wrl_creditt *debit, wrl_creditt debit_floor,
+ wrl_creditt *credit, wrl_creditt credit_ceil)
+ /*
+ * Transfers zero or more credit from "debit" to "credit".
+ * Transfers as much as possible while maintaining
+ * debit >= debit_floor and credit <= credit_ceil.
+ * (If that's violated already, does nothing.)
+ *
+ * Sufficient conditions to avoid overflow, either of:
+ * |every argument| <= 0x3fffffff
+ * |every argument| <= 1E9
+ * |every argument| <= WRL_CREDIT_MAX
+ * (And this condition is preserved.)
+ */
+{
+ wrl_creditt xfer = MIN( *debit - debit_floor,
+ credit_ceil - *credit );
+ if (xfer > 0) {
+ *debit -= xfer;
+ *credit += xfer;
+ }
+}
+
+static void wrl_domain_new(struct domain *domain)
+{
+ domain->wrl_credit = 0;
+ wrl_gettime_now(&domain->wrl_timestamp);
+ wrl_ndomains++;
+ /* Steal up to DBURST from the reserve */
+ wrl_xfer_credit(&wrl_reserve, -wrl_config_newdoms_dburst,
+ &domain->wrl_credit, wrl_config_dburst);
+}
+
+static void wrl_domain_destroy(struct domain *domain)
+{
+ wrl_ndomains--;
+ /*
+ * Don't bother recalculating domain's credit - this just
+ * means we don't give the reserve the ending domain's credit
+ * for time elapsed since last update.
+ */
+ wrl_xfer_credit(&domain->wrl_credit, 0,
+ &wrl_reserve, wrl_config_dburst);
+}
+
+static void wrl_credit_update(struct domain *domain, struct wrl_timestampt now)
+{
+ /*
+ * We want to calculate
+ * credit += (now - timestamp) * RATE / ndoms;
+ * But we want it to saturate, and to avoid floating point.
+ * To avoid rounding errors from constantly adding small
+ * amounts of credit, we only add credit for whole milliseconds.
+ */
+ long seconds = now.sec - domain->wrl_timestamp.sec;
+ long milliseconds = now.msec - domain->wrl_timestamp.msec;
+ long msec;
+ int64_t denom, num;
+ wrl_creditt surplus;
+
+ seconds = MIN(seconds, 1000*1000); /* arbitrary, prevents overflow */
+ msec = seconds * 1000 + milliseconds;
+
+ if (msec < 0)
+ /* shouldn't happen with CLOCK_MONOTONIC */
+ msec = 0;
+
+ /* 32x32 -> 64 cannot overflow */
+ denom = (int64_t)msec * wrl_config_rate;
+ num = (int64_t)wrl_ndomains * 1000;
+ /* denom / num <= 1E6 * wrl_config_rate, so with
+ reasonable wrl_config_rate, denom / num << 2^64 */
+
+ /* at last! */
+ domain->wrl_credit = MIN( (int64_t)domain->wrl_credit + denom / num,
+ WRL_CREDIT_MAX );
+ /* (maybe briefly violating the DBURST cap on wrl_credit) */
+
+ /* maybe take from the reserve to make us nonnegative */
+ wrl_xfer_credit(&wrl_reserve, 0,
+ &domain->wrl_credit, 0);
+
+ /* return any surplus (over DBURST) to the reserve */
+ surplus = 0;
+ wrl_xfer_credit(&domain->wrl_credit, wrl_config_dburst,
+ &surplus, WRL_CREDIT_MAX);
+ wrl_xfer_credit(&surplus, 0,
+ &wrl_reserve, wrl_config_gburst);
+ /* surplus is now implicitly discarded */
+
+ domain->wrl_timestamp = now;
+
+ trace_wrl("dom %4d %6ld msec %9ld credit %9ld reserve %9ld discard\n",
+ domain->domid, msec, (long)domain->wrl_credit,
+ (long)wrl_reserve, (long)surplus);
+}
+
+void wrl_check_timeout(struct domain *domain,
+ struct wrl_timestampt now,
+ int *ptimeout)
+{
+ uint64_t num, denom;
+ int wakeup;
+
+ wrl_credit_update(domain, now);
+
+ if (domain->wrl_credit >= 0)
+ /* not blocked */
+ return;
+
+ if (!*ptimeout)
+ /* already decided on immediate wakeup,
+ so no need to calculate our timeout */
+ return;
+
+ /* calculate wakeup = now + -credit / (RATE / ndoms); */
+
+ /* credit cannot go more -ve than one transaction,
+ * so the first multiplication cannot overflow even 32-bit */
+ num = (uint64_t)(-domain->wrl_credit * 1000) * wrl_ndomains;
+ denom = wrl_config_rate;
+
+ wakeup = MIN( num / denom /* uint64_t */, INT_MAX );
+ if (*ptimeout==-1 || wakeup < *ptimeout)
+ *ptimeout = wakeup;
+
+ trace_wrl("domain %u credit=%ld (reserve=%ld) SLEEPING for %d\n",
+ domain->domid, (long)domain->wrl_credit, (long)wrl_reserve,
+ wakeup);
+}
+
+#define WRL_LOG(now, ...) \
+ (syslog(LOG_WARNING, "write rate limit: " __VA_ARGS__))
+
+void wrl_apply_debit_actual(struct domain *domain)
+{
+ struct wrl_timestampt now;
+
+ if (!domain || !domain_is_unprivileged(domain->conn))
+ /* sockets and privileged domain escape the write rate limit */
+ return;
+
+ wrl_gettime_now(&now);
+ wrl_credit_update(domain, now);
+
+ domain->wrl_credit -= wrl_config_writecost;
+ trace_wrl("domain %u credit=%ld (reserve=%ld)\n", domain->domid,
+ (long)domain->wrl_credit, (long)wrl_reserve);
+
+ if (domain->wrl_credit < 0) {
+ if (!domain->wrl_delay_logged) {
+ domain->wrl_delay_logged = true;
+ WRL_LOG(now, "domain %ld is affected\n",
+ (long)domain->domid);
+ } else if (!wrl_log_last_warning) {
+ WRL_LOG(now, "rate limiting restarts\n");
+ }
+ wrl_log_last_warning = now.sec;
+ }
+}
+
+void wrl_log_periodic(struct wrl_timestampt now)
+{
+ if (wrl_log_last_warning &&
+ (now.sec - wrl_log_last_warning) > WRL_LOGEVERY) {
+ WRL_LOG(now, "not in force recently\n");
+ wrl_log_last_warning = 0;
+ }
+}
+
+void wrl_apply_debit_direct(struct connection *conn)
+{
+ if (!conn)
+ /* some writes are generated internally */
+ return;
+
+ if (conn->transaction)
+ /* these are accounted for when the transaction ends */
+ return;
+
+ if (!wrl_ntransactions)
+ /* we don't conflict with anyone */
+ return;
+
+ wrl_apply_debit_actual(conn->domain);
+}
+
+void wrl_apply_debit_trans_commit(struct connection *conn)
+{
+ if (wrl_ntransactions <= 1)
+ /* our own transaction appears in the counter */
+ return;
+
+ wrl_apply_debit_actual(conn->domain);
+}
+
static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod)
{
return ((prod - cons) <= XENSTORE_RING_SIZE);
@@ -1443,234 +1671,6 @@ unsigned int domain_transaction_get(struct connection *conn)
: 0;
}

-static wrl_creditt wrl_config_writecost = WRL_FACTOR;
-static wrl_creditt wrl_config_rate = WRL_RATE * WRL_FACTOR;
-static wrl_creditt wrl_config_dburst = WRL_DBURST * WRL_FACTOR;
-static wrl_creditt wrl_config_gburst = WRL_GBURST * WRL_FACTOR;
-static wrl_creditt wrl_config_newdoms_dburst =
- WRL_DBURST * WRL_NEWDOMS * WRL_FACTOR;
-
-long wrl_ntransactions;
-
-static long wrl_ndomains;
-static wrl_creditt wrl_reserve; /* [-wrl_config_newdoms_dburst, +_gburst ] */
-static time_t wrl_log_last_warning; /* 0: no previous warning */
-
-#define trace_wrl(...) \
-do { \
- if (trace_flags & TRACE_WRL) \
- trace("wrl: " __VA_ARGS__); \
-} while (0)
-
-void wrl_gettime_now(struct wrl_timestampt *now_wt)
-{
- struct timespec now_ts;
- int r;
-
- r = clock_gettime(CLOCK_MONOTONIC, &now_ts);
- if (r)
- barf_perror("Could not find time (clock_gettime failed)");
-
- now_wt->sec = now_ts.tv_sec;
- now_wt->msec = now_ts.tv_nsec / 1000000;
-}
-
-static void wrl_xfer_credit(wrl_creditt *debit, wrl_creditt debit_floor,
- wrl_creditt *credit, wrl_creditt credit_ceil)
- /*
- * Transfers zero or more credit from "debit" to "credit".
- * Transfers as much as possible while maintaining
- * debit >= debit_floor and credit <= credit_ceil.
- * (If that's violated already, does nothing.)
- *
- * Sufficient conditions to avoid overflow, either of:
- * |every argument| <= 0x3fffffff
- * |every argument| <= 1E9
- * |every argument| <= WRL_CREDIT_MAX
- * (And this condition is preserved.)
- */
-{
- wrl_creditt xfer = MIN( *debit - debit_floor,
- credit_ceil - *credit );
- if (xfer > 0) {
- *debit -= xfer;
- *credit += xfer;
- }
-}
-
-void wrl_domain_new(struct domain *domain)
-{
- domain->wrl_credit = 0;
- wrl_gettime_now(&domain->wrl_timestamp);
- wrl_ndomains++;
- /* Steal up to DBURST from the reserve */
- wrl_xfer_credit(&wrl_reserve, -wrl_config_newdoms_dburst,
- &domain->wrl_credit, wrl_config_dburst);
-}
-
-void wrl_domain_destroy(struct domain *domain)
-{
- wrl_ndomains--;
- /*
- * Don't bother recalculating domain's credit - this just
- * means we don't give the reserve the ending domain's credit
- * for time elapsed since last update.
- */
- wrl_xfer_credit(&domain->wrl_credit, 0,
- &wrl_reserve, wrl_config_dburst);
-}
-
-void wrl_credit_update(struct domain *domain, struct wrl_timestampt now)
-{
- /*
- * We want to calculate
- * credit += (now - timestamp) * RATE / ndoms;
- * But we want it to saturate, and to avoid floating point.
- * To avoid rounding errors from constantly adding small
- * amounts of credit, we only add credit for whole milliseconds.
- */
- long seconds = now.sec - domain->wrl_timestamp.sec;
- long milliseconds = now.msec - domain->wrl_timestamp.msec;
- long msec;
- int64_t denom, num;
- wrl_creditt surplus;
-
- seconds = MIN(seconds, 1000*1000); /* arbitrary, prevents overflow */
- msec = seconds * 1000 + milliseconds;
-
- if (msec < 0)
- /* shouldn't happen with CLOCK_MONOTONIC */
- msec = 0;
-
- /* 32x32 -> 64 cannot overflow */
- denom = (int64_t)msec * wrl_config_rate;
- num = (int64_t)wrl_ndomains * 1000;
- /* denom / num <= 1E6 * wrl_config_rate, so with
- reasonable wrl_config_rate, denom / num << 2^64 */
-
- /* at last! */
- domain->wrl_credit = MIN( (int64_t)domain->wrl_credit + denom / num,
- WRL_CREDIT_MAX );
- /* (maybe briefly violating the DBURST cap on wrl_credit) */
-
- /* maybe take from the reserve to make us nonnegative */
- wrl_xfer_credit(&wrl_reserve, 0,
- &domain->wrl_credit, 0);
-
- /* return any surplus (over DBURST) to the reserve */
- surplus = 0;
- wrl_xfer_credit(&domain->wrl_credit, wrl_config_dburst,
- &surplus, WRL_CREDIT_MAX);
- wrl_xfer_credit(&surplus, 0,
- &wrl_reserve, wrl_config_gburst);
- /* surplus is now implicitly discarded */
-
- domain->wrl_timestamp = now;
-
- trace_wrl("dom %4d %6ld msec %9ld credit %9ld reserve %9ld discard\n",
- domain->domid, msec, (long)domain->wrl_credit,
- (long)wrl_reserve, (long)surplus);
-}
-
-void wrl_check_timeout(struct domain *domain,
- struct wrl_timestampt now,
- int *ptimeout)
-{
- uint64_t num, denom;
- int wakeup;
-
- wrl_credit_update(domain, now);
-
- if (domain->wrl_credit >= 0)
- /* not blocked */
- return;
-
- if (!*ptimeout)
- /* already decided on immediate wakeup,
- so no need to calculate our timeout */
- return;
-
- /* calculate wakeup = now + -credit / (RATE / ndoms); */
-
- /* credit cannot go more -ve than one transaction,
- * so the first multiplication cannot overflow even 32-bit */
- num = (uint64_t)(-domain->wrl_credit * 1000) * wrl_ndomains;
- denom = wrl_config_rate;
-
- wakeup = MIN( num / denom /* uint64_t */, INT_MAX );
- if (*ptimeout==-1 || wakeup < *ptimeout)
- *ptimeout = wakeup;
-
- trace_wrl("domain %u credit=%ld (reserve=%ld) SLEEPING for %d\n",
- domain->domid, (long)domain->wrl_credit, (long)wrl_reserve,
- wakeup);
-}
-
-#define WRL_LOG(now, ...) \
- (syslog(LOG_WARNING, "write rate limit: " __VA_ARGS__))
-
-void wrl_apply_debit_actual(struct domain *domain)
-{
- struct wrl_timestampt now;
-
- if (!domain || !domid_is_unprivileged(domain->domid))
- /* sockets and privileged domain escape the write rate limit */
- return;
-
- wrl_gettime_now(&now);
- wrl_credit_update(domain, now);
-
- domain->wrl_credit -= wrl_config_writecost;
- trace_wrl("domain %u credit=%ld (reserve=%ld)\n", domain->domid,
- (long)domain->wrl_credit, (long)wrl_reserve);
-
- if (domain->wrl_credit < 0) {
- if (!domain->wrl_delay_logged) {
- domain->wrl_delay_logged = true;
- WRL_LOG(now, "domain %ld is affected\n",
- (long)domain->domid);
- } else if (!wrl_log_last_warning) {
- WRL_LOG(now, "rate limiting restarts\n");
- }
- wrl_log_last_warning = now.sec;
- }
-}
-
-void wrl_log_periodic(struct wrl_timestampt now)
-{
- if (wrl_log_last_warning &&
- (now.sec - wrl_log_last_warning) > WRL_LOGEVERY) {
- WRL_LOG(now, "not in force recently\n");
- wrl_log_last_warning = 0;
- }
-}
-
-void wrl_apply_debit_direct(struct connection *conn)
-{
- if (!conn)
- /* some writes are generated internally */
- return;
-
- if (conn->transaction)
- /* these are accounted for when the transaction ends */
- return;
-
- if (!wrl_ntransactions)
- /* we don't conflict with anyone */
- return;
-
- wrl_apply_debit_actual(conn->domain);
-}
-
-void wrl_apply_debit_trans_commit(struct connection *conn)
-{
- if (wrl_ntransactions <= 1)
- /* our own transaction appears in the counter */
- return;
-
- wrl_apply_debit_actual(conn->domain);
-}
-
const char *dump_state_connections(FILE *fp)
{
const char *ret = NULL;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index b9e38abff5..8cd69d42ab 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -162,9 +162,6 @@ struct wrl_timestampt {
extern long wrl_ntransactions;

void wrl_gettime_now(struct wrl_timestampt *now_ts);
-void wrl_domain_new(struct domain *domain);
-void wrl_domain_destroy(struct domain *domain);
-void wrl_credit_update(struct domain *domain, struct wrl_timestampt now);
void wrl_check_timeout(struct domain *domain,
struct wrl_timestampt now,
int *ptimeout);
--
2.35.3
Re: [PATCH v2 05/13] tools/xenstore: make some write limit functions static [ In reply to ]
On 30/03/2023 9:50 am, Juergen Gross wrote:
> +static void wrl_xfer_credit(wrl_creditt *debit, wrl_creditt debit_floor,
> + wrl_creditt *credit, wrl_creditt credit_ceil)
> + /*
> + * Transfers zero or more credit from "debit" to "credit".
> + * Transfers as much as possible while maintaining
> + * debit >= debit_floor and credit <= credit_ceil.
> + * (If that's violated already, does nothing.)
> + *
> + * Sufficient conditions to avoid overflow, either of:
> + * |every argument| <= 0x3fffffff
> + * |every argument| <= 1E9
> + * |every argument| <= WRL_CREDIT_MAX
> + * (And this condition is preserved.)
> + */
> +{
> + wrl_creditt xfer = MIN( *debit - debit_floor,
> + credit_ceil - *credit );

MIN() evaluates its parameters multiple times.  I believe the only legal
way for the compiler to emit this code is to interleave double reads.

As with pretty much any C code, you want to read the pointers into
locals first, then operate on them, then write them out at the end.

~Andrew
Re: [PATCH v2 05/13] tools/xenstore: make some write limit functions static [ In reply to ]
On 03.05.23 16:51, Andrew Cooper wrote:
> On 30/03/2023 9:50 am, Juergen Gross wrote:
>> +static void wrl_xfer_credit(wrl_creditt *debit, wrl_creditt debit_floor,
>> + wrl_creditt *credit, wrl_creditt credit_ceil)
>> + /*
>> + * Transfers zero or more credit from "debit" to "credit".
>> + * Transfers as much as possible while maintaining
>> + * debit >= debit_floor and credit <= credit_ceil.
>> + * (If that's violated already, does nothing.)
>> + *
>> + * Sufficient conditions to avoid overflow, either of:
>> + * |every argument| <= 0x3fffffff
>> + * |every argument| <= 1E9
>> + * |every argument| <= WRL_CREDIT_MAX
>> + * (And this condition is preserved.)
>> + */
>> +{
>> + wrl_creditt xfer = MIN( *debit - debit_floor,
>> + credit_ceil - *credit );
>
> MIN() evaluates its parameters multiple times.  I believe the only legal
> way for the compiler to emit this code is to interleave double reads.
>
> As with pretty much any C code, you want to read the pointers into
> locals first, then operate on them, then write them out at the end.

xenstored is single-threaded. So no need to worry here.


Juergen