Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vchiq: Missing commits from downstream #63

Closed
wants to merge 5 commits into from
Closed

vchiq: Missing commits from downstream #63

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c7884ae
vchiq_arm: Tweak the logging output
Mar 7, 2016
1bc256a
vchiq_arm: Access the dequeue_pending flag locked
Mar 23, 2016
e5afc1b
vchiq_arm: Service callbacks must not fail
Mar 23, 2016
474b510
vchiq_arm: Add completion records under the mutex
Apr 21, 2016
76f2eaf
vchiq_arm: Avoid use of mutex in add_completion
Jun 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 46 additions & 27 deletions drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@
#define VCHIQ_MINOR 0

/* Some per-instance constants */
#define MAX_COMPLETIONS 16
#define MAX_COMPLETIONS 128
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit messages doesn't explain why it's increased. So this should be a separate patch.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pelwell Thanks for the other comment, but the changes to MAX_COMPLETIONS and MSG_QUEUE_SIZE needs a separate commit message, too.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vchiq_arm: Avoid premature stalls

The constants MAX_COMPLETIONS and MSG_QUEUE_SIZE control the number of
messages that can be outstanding to each client before the system as a
whole stalls. If the numbers are too small then unnecessary thread
switching will occur while waiting for a (potentially low priority)
client thread to consume some data; badly written clients can even
lead to deadlock.

For services that carry many short messages, 16 messages can represent
a very small amount of data. Since the resources are small - 16 bytes
for a completion, 4 bytes for a message pointer - increase the limits
so they are unlikely to be hit except in exceptional circumstances.

Signed-off-by: Phil Elwell [email protected]

#define MAX_SERVICES 64
#define MAX_ELEMENTS 8
#define MSG_QUEUE_SIZE 64
#define MSG_QUEUE_SIZE 128

#define KEEPALIVE_VER 1
#define KEEPALIVE_VER_MIN KEEPALIVE_VER
Expand Down Expand Up @@ -208,30 +208,32 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason,
void *bulk_userdata)
{
VCHIQ_COMPLETION_DATA_T *completion;
int insert;
DEBUG_INITIALISE(g_state.local)

while (instance->completion_insert ==
(instance->completion_remove + MAX_COMPLETIONS)) {
insert = instance->completion_insert;
while ((insert - instance->completion_remove) >= MAX_COMPLETIONS) {
/* Out of space - wait for the client */
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
vchiq_log_trace(vchiq_arm_log_level,
"add_completion - completion queue full");
DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);

if (down_interruptible(&instance->remove_event) != 0) {
vchiq_log_info(vchiq_arm_log_level,
"service_callback interrupted");
return VCHIQ_RETRY;
} else if (instance->closing) {
}

if (instance->closing) {
vchiq_log_info(vchiq_arm_log_level,
"service_callback closing");
return VCHIQ_ERROR;
return VCHIQ_SUCCESS;
}
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
}

completion =
&instance->completions[instance->completion_insert &
(MAX_COMPLETIONS - 1)];
completion = &instance->completions[insert & (MAX_COMPLETIONS - 1)];

completion->header = header;
completion->reason = reason;
Expand All @@ -252,9 +254,9 @@ add_completion(VCHIQ_INSTANCE_T instance, VCHIQ_REASON_T reason,
wmb();

if (reason == VCHIQ_MESSAGE_AVAILABLE)
user_service->message_available_pos =
instance->completion_insert;
instance->completion_insert++;
user_service->message_available_pos = insert;

instance->completion_insert = ++insert;

up(&instance->insert_event);

Expand All @@ -279,6 +281,7 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
USER_SERVICE_T *user_service;
VCHIQ_SERVICE_T *service;
VCHIQ_INSTANCE_T instance;
int skip_completion = 0;
DEBUG_INITIALISE(g_state.local)

DEBUG_TRACE(SERVICE_CALLBACK_LINE);
Expand Down Expand Up @@ -345,9 +348,6 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
user_service->msg_queue[user_service->msg_insert &
(MSG_QUEUE_SIZE - 1)] = header;
user_service->msg_insert++;
spin_unlock(&msg_queue_spinlock);

up(&user_service->insert_event);

/* If there is a thread waiting in DEQUEUE_MESSAGE, or if
** there is a MESSAGE_AVAILABLE in the completion queue then
Expand All @@ -356,13 +356,22 @@ service_callback(VCHIQ_REASON_T reason, VCHIQ_HEADER_T *header,
if (((user_service->message_available_pos -
instance->completion_remove) >= 0) ||
user_service->dequeue_pending) {
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
user_service->dequeue_pending = 0;
return VCHIQ_SUCCESS;
skip_completion = 1;
}

spin_unlock(&msg_queue_spinlock);

up(&user_service->insert_event);

header = NULL;
}

if (skip_completion) {
DEBUG_TRACE(SERVICE_CALLBACK_LINE);
return VCHIQ_SUCCESS;
}

DEBUG_TRACE(SERVICE_CALLBACK_LINE);

return add_completion(instance, reason, header, user_service,
Expand Down Expand Up @@ -777,6 +786,7 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
instance->completion_insert)
&& !instance->closing) {
int rc;

DEBUG_TRACE(AWAIT_COMPLETION_LINE);
mutex_unlock(&instance->completion_mutex);
rc = down_interruptible(&instance->insert_event);
Expand All @@ -791,24 +801,29 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
}
DEBUG_TRACE(AWAIT_COMPLETION_LINE);

/* A read memory barrier is needed to stop prefetch of a stale
** completion record
*/
rmb();

if (ret == 0) {
int msgbufcount = args.msgbufcount;
int remove;

remove = instance->completion_remove;

for (ret = 0; ret < args.count; ret++) {
VCHIQ_COMPLETION_DATA_T *completion;
VCHIQ_SERVICE_T *service;
USER_SERVICE_T *user_service;
VCHIQ_HEADER_T *header;
if (instance->completion_remove ==
instance->completion_insert)

if (remove == instance->completion_insert)
break;

completion = &instance->completions[
instance->completion_remove &
(MAX_COMPLETIONS - 1)];
remove & (MAX_COMPLETIONS - 1)];


/* A read memory barrier is needed to prevent
** the prefetch of a stale completion record
*/
rmb();

service = completion->service_userdata;
user_service = service->base.userdata;
Expand Down Expand Up @@ -885,7 +900,11 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
break;
}

instance->completion_remove++;
/* Ensure that the above copy has completed
** before advancing the remove pointer. */
mb();

instance->completion_remove = ++remove;
}

if (msgbufcount != args.msgbufcount) {
Expand Down
45 changes: 22 additions & 23 deletions drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,22 +610,24 @@ process_free_queue(VCHIQ_STATE_T *state)
BITSET_T service_found[BITSET_SIZE(VCHIQ_MAX_SERVICES)];
int slot_queue_available;

/* Use a read memory barrier to ensure that any state that may have
** been modified by another thread is not masked by stale prefetched
** values. */
rmb();

/* Find slots which have been freed by the other side, and return them
** to the available queue. */
slot_queue_available = state->slot_queue_available;

/* Use a memory barrier to ensure that any state that may have been
** modified by another thread is not masked by stale prefetched
** values. */
mb();

while (slot_queue_available != local->slot_queue_recycle) {
unsigned int pos;
int slot_index = local->slot_queue[slot_queue_available++ &
VCHIQ_SLOT_QUEUE_MASK];
char *data = (char *)SLOT_DATA_FROM_INDEX(state, slot_index);
int data_found = 0;

rmb();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pelwell I've rebased and rearranged this series. But checkpatch complains about a missing comment why this read memory barrier is necessary.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

	/* Beware of the address dependency - data is calculated
	** using an index written by the other side. */


vchiq_log_trace(vchiq_core_log_level, "%d: pfq %d=%x %x %x",
state->id, slot_index, (unsigned int)data,
local->slot_queue_recycle, slot_queue_available);
Expand Down Expand Up @@ -741,6 +743,8 @@ process_free_queue(VCHIQ_STATE_T *state)
up(&state->data_quota_event);
}

mb();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pelwell checkpatch complains here, too

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

		/* Don't allow the slot to be reused until we are no
		** longer interested in it. */


state->slot_queue_available = slot_queue_available;
up(&state->slot_available_event);
}
Expand Down Expand Up @@ -891,16 +895,14 @@ queue_message(VCHIQ_STATE_T *state, VCHIQ_SERVICE_T *service,
error_count);
return VCHIQ_ERROR;
}
if (i == 0) {
if (SRVTRACE_ENABLED(service,
VCHIQ_LOG_INFO))
vchiq_log_dump_mem("Sent", 0,
header->data + pos,
min(64u,
elements[0].size));
}
}

if (SRVTRACE_ENABLED(service,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a commit message which explain why this complete patch is necessary.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Move the trace message outside the gather loop so fragmentation
    doesn't mask significant bytes.
  • Reduce the size of the data logged - 16 bytes is sufficient.

Signed-off-by: Phil Elwell [email protected]

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. It seems that the first part has already been applied.

VCHIQ_LOG_INFO))
vchiq_log_dump_mem("Sent", 0,
header->data,
min(16, pos));

spin_lock(&quota_spinlock);
service_quota->message_use_count++;

Expand Down Expand Up @@ -1039,16 +1041,13 @@ queue_message_sync(VCHIQ_STATE_T *state, VCHIQ_SERVICE_T *service,
error_count);
return VCHIQ_ERROR;
}
if (i == 0) {
if (vchiq_sync_log_level >=
VCHIQ_LOG_TRACE)
vchiq_log_dump_mem("Sent Sync",
0, header->data + pos,
min(64u,
elements[0].size));
}
}

if (vchiq_sync_log_level >= VCHIQ_LOG_TRACE)
vchiq_log_dump_mem("Sent Sync",
0, header->data,
min(16, pos));

VCHIQ_SERVICE_STATS_INC(service, ctrl_tx_count);
VCHIQ_SERVICE_STATS_ADD(service, ctrl_tx_bytes, size);
} else {
Expand Down Expand Up @@ -1720,7 +1719,7 @@ parse_rx_slots(VCHIQ_STATE_T *state)
remoteport, localport, size);
if (size > 0)
vchiq_log_dump_mem("Rcvd", 0, header->data,
min(64, size));
min(16, size));
}

if (((unsigned int)header & VCHIQ_SLOT_MASK) + calc_stride(size)
Expand Down Expand Up @@ -2187,7 +2186,7 @@ sync_func(void *v)
remoteport, localport, size);
if (size > 0)
vchiq_log_dump_mem("Rcvd", 0, header->data,
min(64, size));
min(16, size));
}

switch (type) {
Expand Down