Hi all,
the ISDN kernelcapi function recv_handler() is triggered by
schedule_work() and dispatches the CAPI messages to the applications.
Since a workqueue function may run on another CPU at the same time,
reordering of CAPI messages may occur.
For serialization I suggest a mutex semaphore in recv_handler(),
patch is appended (yet untested).
Is there a better way to do user-context work serialized ?
Armin
--- linux/drivers/isdn/capi/kcapi.c 16 Mar 2004 08:01:47 -0000
+++ linux/drivers/isdn/capi/kcapi.c 18 Mar 2004 10:41:38 -0000
@@ -70,6 +70,7 @@
static struct sk_buff_head recv_queue;
static struct work_struct tq_recv_notify;
+static DECLARE_MUTEX(recv_handler_lock);
/* -------- controller ref counting -------------------------------------- */
@@ -242,6 +243,8 @@
struct sk_buff *skb;
struct capi20_appl *ap;
+ down(&recv_handler_lock);
+
while ((skb = skb_dequeue(&recv_queue)) != 0) {
ap = get_capi_appl_by_nr(CAPIMSG_APPID(skb->data));
if (!ap) {
@@ -257,6 +260,8 @@
ap->nrecvctlpkt++;
ap->recv_message(ap, skb);
}
+
+ up(&recv_handler_lock);
}
void capi_ctr_handle_message(struct capi_ctr * card, u16 appl, struct sk_buff *skb)
Armin Schindler <[email protected]> wrote:
>
> Hi all,
>
> the ISDN kernelcapi function recv_handler() is triggered by
> schedule_work() and dispatches the CAPI messages to the applications.
>
> Since a workqueue function may run on another CPU at the same time,
> reordering of CAPI messages may occur.
TCP has the same problem.
> For serialization I suggest a mutex semaphore in recv_handler(),
> patch is appended (yet untested).
It will work OK. It isn't very scalable of course, but I assume you're
dealing with relatively low bandwidths.
I would suggest that you look at avoiding the global semaphore. Suppose
someone has 64 interfaces or something. Is that possible? It might be
better to put the semaphore into struct capi_ctr so you can at least
process frames from separate cards in parallel.
> Is there a better way to do user-context work serialized ?
Not really - you've been bitten by the compulsory per-cpuness of the
workqueue handlers.
You could have a standalone kernel thread or always queue the work onto CPU
#0 (the function to do this isn't merged, but exists). But both these are
unscalable.
So apart from moving recv_handler_lock into struct capi_ctr I can't think
of anything clever.
On Thu, 18 Mar 2004, Andrew Morton wrote:
> Armin Schindler <[email protected]> wrote:
> >
> > Hi all,
> >
> > the ISDN kernelcapi function recv_handler() is triggered by
> > schedule_work() and dispatches the CAPI messages to the applications.
> >
> > Since a workqueue function may run on another CPU at the same time,
> > reordering of CAPI messages may occur.
>
> TCP has the same problem.
>
> > For serialization I suggest a mutex semaphore in recv_handler(),
> > patch is appended (yet untested).
>
> It will work OK. It isn't very scalable of course, but I assume you're
> dealing with relatively low bandwidths.
Right, compared with network, I guess it is low bandwith.
> I would suggest that you look at avoiding the global semaphore. Suppose
> someone has 64 interfaces or something. Is that possible? It might be
> better to put the semaphore into struct capi_ctr so you can at least
> process frames from separate cards in parallel.
This was just mentioned on the i4l-developer list too.
But not on capi_ctr basis, a semaphore per application (struct capi20_appl)
should be better.
The reason for the serialization is a possible re-ordering of messages
per application, so only the application needs the semaphore.
Thanks Andrew, I will prepare a new patch.
Armin
On Thu, 18 Mar 2004, Andrew Morton wrote:
> I would suggest that you look at avoiding the global semaphore. Suppose
> someone has 64 interfaces or something. Is that possible? It might be
> better to put the semaphore into struct capi_ctr so you can at least
> process frames from separate cards in parallel.
>
> > Is there a better way to do user-context work serialized ?
>
> Not really - you've been bitten by the compulsory per-cpuness of the
> workqueue handlers.
>
> You could have a standalone kernel thread or always queue the work onto CPU
> #0 (the function to do this isn't merged, but exists). But both these are
> unscalable.
>
> So apart from moving recv_handler_lock into struct capi_ctr I can't think
> of anything clever.
I think an atomic counter in the workqueue-function would be more efficient.
What do you think about this?
static atomic_t recv_work_count;
static void recv_handler(void *data)
{
if (atomic_inc_and_test(&recv_work_count)) {
do {
/* work to do */
} while (!atomic_add_negative(-1, &recv_work_count));
}
}
static int __init kcapi_init(void)
{
atomic_set(&recv_work_count, -1);
}
A second call to the recv_handler() just results in an additional loop
of the first one and the second CPU goes on with its work.
Unfortunately, the atomic functions are not available on all architechtures.
atomic_dec_and_test() is the only atomic function with return value on all
platforms, right? Which isn't enough for the loop above.
Armin
Armin Schindler <[email protected]> wrote:
>
> On Thu, 18 Mar 2004, Andrew Morton wrote:
> > I would suggest that you look at avoiding the global semaphore. Suppose
> > someone has 64 interfaces or something. Is that possible? It might be
> > better to put the semaphore into struct capi_ctr so you can at least
> > process frames from separate cards in parallel.
> >
> > > Is there a better way to do user-context work serialized ?
> >
> > Not really - you've been bitten by the compulsory per-cpuness of the
> > workqueue handlers.
> >
> > You could have a standalone kernel thread or always queue the work onto CPU
> > #0 (the function to do this isn't merged, but exists). But both these are
> > unscalable.
> >
> > So apart from moving recv_handler_lock into struct capi_ctr I can't think
> > of anything clever.
>
> I think an atomic counter in the workqueue-function would be more efficient.
> What do you think about this?
>
>
> static atomic_t recv_work_count;
>
> static void recv_handler(void *data)
> {
> if (atomic_inc_and_test(&recv_work_count)) {
> do {
> /* work to do */
> } while (!atomic_add_negative(-1, &recv_work_count));
> }
> }
>
> static int __init kcapi_init(void)
> {
> atomic_set(&recv_work_count, -1);
> }
>
>
> A second call to the recv_handler() just results in an additional loop
> of the first one and the second CPU goes on with its work.
It doesn't need to be a counter - it can simply be a flag. See how (say)
queue_work() does it.
However you are limiting things so only a single CPU can run `work to do'
at any time, same as with a semaphore.
On Sat, 20 Mar 2004, Andrew Morton wrote:
> Armin Schindler <[email protected]> wrote:
> >
> > static atomic_t recv_work_count;
> >
> > static void recv_handler(void *data)
> > {
> > if (atomic_inc_and_test(&recv_work_count)) {
> > do {
> > /* work to do */
> > } while (!atomic_add_negative(-1, &recv_work_count));
> > }
> > }
> >
> > static int __init kcapi_init(void)
> > {
> > atomic_set(&recv_work_count, -1);
> > }
> >
> >
> > A second call to the recv_handler() just results in an additional loop
> > of the first one and the second CPU goes on with its work.
>
> It doesn't need to be a counter - it can simply be a flag. See how (say)
> queue_work() does it.
>
> However you are limiting things so only a single CPU can run `work to do'
> at any time, same as with a semaphore.
Well, limiting the 'work to do' to one CPU is exactly what I need to do,
the code must not run on another CPU at the same time.
If just a 'flag' is used, it is possible to loose an event, the flag like in
queue_work() prevents from running twice only.
But if the first work is just finished and about to reset the flag, the
check for the flag in the second CPU results to do nothing and a possible
needed work is not done.
The idea of using an atomic counter instead of the semaphore is for
performance reasons. It's better if the second work-to-do just increments
a counter, which the first work handles afterwards, than put this thread to
sleep.
Armin
Armin Schindler <[email protected]> wrote:
>
> > However you are limiting things so only a single CPU can run `work to do'
> > at any time, same as with a semaphore.
>
> Well, limiting the 'work to do' to one CPU is exactly what I need to do,
> the code must not run on another CPU at the same time.
Across the entire kernel? For *all* ISDN connections and interfaces?
Surely there must be some finer-grained level at which the serialisation is
needed - per-inteface, per-connection, per-session, whatever?
On Sat, 20 Mar 2004, Andrew Morton wrote:
> Armin Schindler <[email protected]> wrote:
> >
> > > However you are limiting things so only a single CPU can run `work to do'
> > > at any time, same as with a semaphore.
> >
> > Well, limiting the 'work to do' to one CPU is exactly what I need to do,
> > the code must not run on another CPU at the same time.
>
> Across the entire kernel? For *all* ISDN connections and interfaces?
Actually yes, for incoming messages.
> Surely there must be some finer-grained level at which the serialisation is
> needed - per-inteface, per-connection, per-session, whatever?
All incoming CAPI messages are queued in one global list and then the work
is scheduled. The recv_handler() is just a dispatcher, which dequeues all
new messages and puts them into the right queue for the application to read.
"the code must not run on another CPU at the same time" is not quite
correct. The code itself may run more than once of course, just the
dequeue-from-global-queue and put into application-queue sequence may
not run twice for the same application.
Armin