Re: [PATCH RFC 10/20] ctlv: Prepare event queue support

[Yeoreum Yun]

Hi Jiri,


[...]

> +int ctlv_event_emit(struct ctlv_op_ctx *ctx)
> +{
> +	struct ctlv_device *ctlvdev = ctx->ctlvdev;
> +	struct ctlv_event_record *rec, *next;
> +	struct ctlv_file *cfile;
> +	u32 len, wanted = 0;
> +	LIST_HEAD(spare);
> +	int ret;
> +
> +	if (WARN_ON_ONCE(ctx->file))
> +		return -EINVAL;
> +	ret = ctlv_event_finish(ctx);
> +	if (ret)
> +		goto out_free;
> +
> +	len = sizeof(struct ctlv_event_hdr) + CTLV_ALIGN(ctx->output.root->len);
> +	scoped_guard(spinlock, &ctlvdev->events_lock) {
> +		if (ctlvdev->dead_announced)
> +			goto out_free;
> +		list_for_each_entry(cfile, &ctlvdev->files, node)
> +			if (test_bit(ctx->op->event_index, cfile->subscribed))
> +				wanted++;
> +	}

Suppose at the time of here, the number of subscriber is 1.
However, before calling the ctlv_event_deliver(), another task hold the
event_lock() to register new subscriber. for example:


Task 1 (emit)					Task 2 (new subscriber B)

wanted = 1
unlock events_lock.
						Register subscription.
allocate one record
ctlv_event_delver()

  grab events_lock.
    Deliver former subscribe event to old one.
    new subscriber B will call ctlv_event_queue_event() with NULL.

When this happens, new subscriber B would receive overflow event
via ctlv_event_queue_dropped() though it was registered right before.

Is this intended? It doesn't make sense that subscribe B receives
the overflow event in this situation...


> +	if (wanted) {
> +		ctlv_event_copies(ctx, len, &spare, wanted);
> +		ctlv_event_deliver(ctx, &spare);
> +		list_for_each_entry_safe(rec, next, &spare, node)
> +			kfree(rec);
> +	}
> +
> +out_free:
> +	ctlv_event_free(ctx);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(ctlv_event_emit, "CTLV");
> +

[...]

Thanks!

-- 
Sincerely,
Yeoreum Yun