freeradius-client sequence file replacement
Hello, I made a patch that adds fast locking mechanisms instead of seq file for freeradius-client. This patch is based on putting the sequence number in the shared memory. Two approaches used to do a mutual exclusion for shared memory: a) GCC builtin atomic memory access (only on Intel) b) POSIX named semaphore Fallback: c) If nothing of the above available it fallbacks to a sequence file locks. Originally I did it against radiusclient-ng, but looks that freeradius-client is a correct place. The patch works for freeradius-client 1.1.6 as well. Here's a patch and more detailed description: http://developer.berlios.de/patch/?func=detailpatch&patch_id=2810&group_id=1... -- Best Regards, Alex Massover VoIP R&D TL Jajah Inc. This mail was sent via Mail-SeCure System.
Alex Massover wrote:
I made a patch that adds fast locking mechanisms instead of seq file for freeradius-client.
OK... I just re-read the original code, and boggled a bit. Using a file for a sequence number is completely wrong. There is just no need. I'd suggest two patches: 1) replace "seq_nbr" in the code with "id". The field is NOT a sequence number. it's a random id. The ONLY requirement is that it's unique until the outgoing packet times out, or a reply is received. 2) replace the rc_get_seqnbr() with a function that tracks 256 allocated IDs, and allocates a free one. Also update rc_check_reply() to mark the "id" as free. This will be *enormously* faster than using a file or shared memory. The array of 256 Ids needs to be unique per source port. But it can be shared across child threads. It does NOT need to be shared across multiple processes. The main server code does it a little differently. See src/lib/packet.c for a number of functions that track/allocate/free packet Ids. Alan DeKok.
Hi, What is supposed to happen if there's no free id available?
2) replace the rc_get_seqnbr() with a function that tracks 256 allocated IDs, and allocates a free one. Also update rc_check_reply() to mark the "id" as free.
This will be *enormously* faster than using a file or shared memory.
The array of 256 Ids needs to be unique per source port. But it can be shared across child threads. It does NOT need to be shared across multiple processes.
The main server code does it a little differently. See src/lib/packet.c for a number of functions that track/allocate/free packet Ids.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
This mail was received via Mail-SeCure System.
This mail was sent via Mail-SeCure System.
Alex Massover wrote:
What is supposed to happen if there's no free id available?
The client opens a new source port, and has 256 free Id's. Also, the Ids should be tracked by code, too. If there's an Access-Request with Id 1, then there can *also* be an Accounting-Request with Id 1. i.e. each source port can have 256 IDs for Access-Request, and another (independent) 256 IDs for Accounting-Request. Alan DeKok.
Hi, That requires a redesign of the way freeradius-client works, because currently it opens new socket for every request. What do you think about continuing in two steps, short term and long term? Now seq file really limits performance, so for short term I can just drop rc_get_seqnbr() in favor of random number (random code already exists there). Anyway it doesn't matter what id will be there because new socket will be used. For a long term maybe it worth implementing what you described below, or maybe it's good enough to stay with new socket?
-----Original Message----- From: freeradius-devel-bounces+alex=jajah.com@lists.freeradius.org [mailto:freeradius-devel-bounces+alex=jajah.com@lists.freeradius.org] On Behalf Of Alan DeKok Sent: יום ב 01 פברואר 2010 13:32 To: FreeRadius developers mailing list Subject: Re: freeradius-client sequence file replacement
Alex Massover wrote:
What is supposed to happen if there's no free id available?
The client opens a new source port, and has 256 free Id's.
Also, the Ids should be tracked by code, too. If there's an Access-Request with Id 1, then there can *also* be an Accounting- Request with Id 1.
i.e. each source port can have 256 IDs for Access-Request, and another (independent) 256 IDs for Accounting-Request.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
This mail was received via Mail-SeCure System.
This mail was sent via Mail-SeCure System.
Alex Massover wrote:
That requires a redesign of the way freeradius-client works, because currently it opens new socket for every request.
Then set the ID field (seq_nbr) to a random number.
Now seq file really limits performance, so for short term I can just drop rc_get_seqnbr() in favor of random number (random code already exists there). Anyway it doesn't matter what id will be there because new socket will be used.
Yes. Set it to a random number. Even rand() here is good enough.
For a long term maybe it worth implementing what you described below, or maybe it's good enough to stay with new socket?
For high performance systems, having multiple packets on one socket would be best. Alan DeKok.
Here's a patch removing seq file. Also there's no need in lib/lock.c anymore. diff -ur ./BUGS /tmp/freeradius-client-1.1.6/BUGS --- ./BUGS 2007-06-21 21:35:00.000000000 +0300 +++ /tmp/freeradius-client-1.1.6/BUGS 2010-02-01 16:54:25.203211724 +0200 @@ -3,12 +3,6 @@ Testing can show the presense of bugs, but not their absence. -- Dijkstra -The get_seqnbr function in build_req.c sometimes returns the same -number, if invoked from multiple processes at the same time. This -occurs only if a lot of process try to read the sequence file, -so I suspect a race condition in the locking code, but I can't -find one. Any help is appreciated. - Radlogin respectively send_server in sendserver.c does not honour a ACCESS_CHALLENGE packet from the RADIUS server. diff -ur ./include/freeradius-client.h /tmp/freeradius-client-1.1.6/include/freeradius-client.h --- ./include/freeradius-client.h 2008-02-11 08:54:23.000000000 +0200 +++ /tmp/freeradius-client-1.1.6/include/freeradius-client.h 2010-02-01 16:47:17.084878041 +0200 @@ -426,7 +426,7 @@ /* buildreq.c */ void rc_buildreq(rc_handle *, SEND_DATA *, int, char *, unsigned short, char *, int, int); -unsigned char rc_get_seqnbr(rc_handle *); +unsigned char rc_get_id(); int rc_auth(rc_handle *, uint32_t, VALUE_PAIR *, VALUE_PAIR **, char *); int rc_auth_proxy(rc_handle *, VALUE_PAIR *, VALUE_PAIR **, char *); int rc_acct(rc_handle *, uint32_t, VALUE_PAIR *); diff -ur ./lib/buildreq.c /tmp/freeradius-client-1.1.6/lib/buildreq.c --- ./lib/buildreq.c 2008-03-05 18:35:20.000000000 +0200 +++ /tmp/freeradius-client-1.1.6/lib/buildreq.c 2010-02-01 17:29:46.987179691 +0200 @@ -13,7 +13,7 @@ #include <includes.h> #include <freeradius-client.h> -unsigned char rc_get_seqnbr(rc_handle *); +unsigned char rc_get_id(); /* * Function: rc_buildreq @@ -29,87 +29,26 @@ data->server = server; data->secret = secret; data->svc_port = port; - data->seq_nbr = rc_get_seqnbr(rh); + data->seq_nbr = rc_get_id(); data->timeout = timeout; data->retries = retries; data->code = code; } /* - * Function: rc_guess_seqnbr + * Function: rc_get_id * - * Purpose: return a random sequence number + * Purpose: generate random id * */ -static unsigned char rc_guess_seqnbr(void) +unsigned char rc_get_id() { srandom((unsigned int)(time(NULL)+getpid())); return (unsigned char)(random() & UCHAR_MAX); } /* - * Function: rc_get_seqnbr - * - * Purpose: generate a sequence number - * - */ - -unsigned char rc_get_seqnbr(rc_handle *rh) -{ - FILE *sf; - int tries = 1; - int seq_nbr; - char *seqfile = rc_conf_str(rh, "seqfile"); - - if ((sf = fopen(seqfile, "a+")) == NULL) - { - rc_log(LOG_ERR,"rc_get_seqnbr: couldn't open sequence file %s: %s", seqfile, strerror(errno)); - /* well, so guess a sequence number */ - return rc_guess_seqnbr(); - } - - while (do_lock_exclusive(sf)!= 0) - { - if (errno != EWOULDBLOCK) { - rc_log(LOG_ERR, "rc_get_seqnbr: flock failure: %s: %s", seqfile, strerror(errno)); - fclose(sf); - return rc_guess_seqnbr(); - } - tries++; - if (tries <= 10) - rc_mdelay(500); - else - break; - } - - if (tries > 10) { - rc_log(LOG_ERR,"rc_get_seqnbr: couldn't get lock after %d tries: %s", tries-1, seqfile); - fclose(sf); - return rc_guess_seqnbr(); - } - - rewind(sf); - if (fscanf(sf, "%d", &seq_nbr) != 1) { - rc_log(LOG_ERR,"rc_get_seqnbr: fscanf failure: %s", seqfile); - seq_nbr = rc_guess_seqnbr(); - } - - rewind(sf); - ftruncate(fileno(sf),0); - fprintf(sf,"%d\n", (seq_nbr+1) & UCHAR_MAX); - - fflush(sf); /* fflush because a process may read it between the do_unlock and fclose */ - - if (do_unlock(sf) != 0) - rc_log(LOG_ERR, "rc_get_seqnbr: couldn't release lock on %s: %s", seqfile, strerror(errno)); - - fclose(sf); - - return (unsigned char)seq_nbr; -} - -/* * Function: rc_aaa * * Purpose: Builds an authentication/accounting request for port id client_port
-----Original Message----- From: freeradius-devel-bounces+alex=jajah.com@lists.freeradius.org [mailto:freeradius-devel-bounces+alex=jajah.com@lists.freeradius.org] On Behalf Of Alan DeKok Sent: יום ב 01 פברואר 2010 15:20 To: FreeRadius developers mailing list Subject: Re: freeradius-client sequence file replacement
Alex Massover wrote:
That requires a redesign of the way freeradius-client works, because currently it opens new socket for every request.
Then set the ID field (seq_nbr) to a random number.
Now seq file really limits performance, so for short term I can just drop rc_get_seqnbr() in favor of random number (random code already exists there). Anyway it doesn't matter what id will be there because new socket will be used.
Yes. Set it to a random number. Even rand() here is good enough.
For a long term maybe it worth implementing what you described below, or maybe it's good enough to stay with new socket?
For high performance systems, having multiple packets on one socket would be best.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
This mail was received via Mail-SeCure System.
This mail was sent via Mail-SeCure System.
Sure, sorry, attached.
-----Original Message----- From: freeradius-devel-bounces+alex=jajah.com@lists.freeradius.org [mailto:freeradius-devel-bounces+alex=jajah.com@lists.freeradius.org] On Behalf Of Alan DeKok Sent: יום ג 02 פברואר 2010 17:51 To: FreeRadius developers mailing list Subject: Re: freeradius-client sequence file replacement
Alex Massover wrote:
Here's a patch removing seq file. Also there's no need in lib/lock.c anymore.
OK. Can you send it as an attachment, so that the mailer doesn't wrap long lines?
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
This mail was received via Mail-SeCure System.
This mail was sent via Mail-SeCure System.
participants (2)
-
Alan DeKok -
Alex Massover