Re: [PATCH, post-03/20 1/1] xfs: hook up inodegc to CPU dead notification

On Wed, Aug 04, 2021 at 09:52:25PM +1000, Dave Chinner wrote:

unsigned int, since that's the caller's type.

+{
+	struct xfs_inodegc	*dead_gc, *gc;
+	struct llist_node	*first, *last;
+	int			count = 0;
+
+	dead_gc = per_cpu_ptr(mp->m_inodegc, dead_cpu);
+	cancel_work_sync(&dead_gc->work);
+
+	if (llist_empty(&dead_gc->list))
+		return;
+
+	first = dead_gc->list.first;
+	last = first;
+	while (last->next) {
+		last = last->next;
+		count++;
+	}
+	dead_gc->list.first = NULL;
+	dead_gc->items = 0;
+
+	/* Add pending work to current CPU */
+	gc = get_cpu_ptr(mp->m_inodegc);
+	llist_add_batch(first, last, &gc->list);
+	count += READ_ONCE(gc->items);
+	WRITE_ONCE(gc->items, count);

I was wondering about the READ/WRITE_ONCE pattern for gc->items: it's
meant to be an accurate count of the list items, right?  But there's no
hard synchronization (e.g. spinlock) around them, which means that the
only CPU that can access that variable at all is the one that the percpu
structure belongs to, right?  And I think that's ok here, because the
only accessors are _queue() and _worker(), which both are supposed to
run on the same CPU since they're percpu lists, right?

In which case: why can't we just say count = dead_gc->items;?  @dead_cpu
is being offlined, which implies that nothing will get scheduled on it,
right?

+	put_cpu_ptr(gc);
+	queue_work(mp->m_inodegc_wq, &gc->work);

Should this be thresholded like we do for _inodegc_queue?

In the old days I would have imagined that cpu offlining should be rare
enough <cough> that it probably doesn't make any real difference.  OTOH
my cloudic colleague reminds me that they aggressively offline cpus to
reduce licensing cost(!).

--D