| Message ID | 20081220080813.GC3853@gerrit.erg.abdn.ac.uk |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Em Sat, Dec 20, 2008 at 09:08:13AM +0100, Gerrit Renker escreveu: > dccp: Lockless use of CCIDs > > This updates the implementation to use only a single array whose size > equals the number of configured CCIDs instead of 255. > > Since the CCIDs are fixed array elements, synchronization is no longer > needed. > > Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk> > --- > net/dccp/ccid.h | 10 --- > net/dccp/ccid.c | 166 +++++++++++--------------------------------------------- > net/dccp/feat.c | 2 > 3 files changed, 38 insertions(+), 140 deletions(-) > > --- a/net/dccp/ccid.h > +++ b/net/dccp/ccid.h > @@ -19,14 +19,12 @@ > #include <linux/list.h> > #include <linux/module.h> > > -#define CCID_MAX 255 > - > struct tcp_info; > > /** > * struct ccid_operations - Interface to Congestion-Control Infrastructure > * > - * @ccid_id: numerical CCID ID (up to %CCID_MAX, cf. table 5 in RFC 4340, 10.) > + * @ccid_id: numerical CCID ID (cf. table 5 in RFC 4340, 10.) > * @ccid_ccmps: the CCMPS including network/transport headers (0 when disabled) > * @ccid_name: alphabetical identifier string for @ccid_id > * @ccid_hc_{r,t}x_slab: memory pool for the receiver/sender half-connection > @@ -93,9 +91,6 @@ extern struct ccid_operations ccid2_ops; > extern struct ccid_operations ccid3_ops; > #endif > > -extern int ccid_register(struct ccid_operations *ccid_ops); > -extern int ccid_unregister(struct ccid_operations *ccid_ops); > - > extern int ccids_register_builtins(void); > > struct ccid { > @@ -113,8 +108,7 @@ extern int ccid_get_builtin_ccids(u8 ** > extern int ccid_getsockopt_builtin_ccids(struct sock *sk, int len, > char __user *, int __user *); > > -extern struct ccid *ccid_new(unsigned char id, struct sock *sk, int rx, > - gfp_t gfp); > +extern struct ccid *ccid_new(const u8 id, struct sock *sk, bool rx); > > static inline int ccid_get_current_rx_ccid(struct dccp_sock *dp) > { > --- a/net/dccp/ccid.c > +++ b/net/dccp/ccid.c > @@ -13,8 +13,8 @@ > > #include "ccid.h" > > -static struct ccid_operations *builtin_ccids_ops[] = { > - &ccid2_ops, /* CCID2 is supported by default */ > +static struct ccid_operations *ccids[] = { > + &ccid2_ops, /* CCID-2 is supported by default */ > #ifdef CONFIG_IP_DCCP_CCID3 > &ccid3_ops, > #endif > @@ -27,49 +27,6 @@ static u8 builtin_ccids[] = { > #endif > }; > > -static struct ccid_operations *ccids[CCID_MAX]; > -#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT) > -static atomic_t ccids_lockct = ATOMIC_INIT(0); > -static DEFINE_SPINLOCK(ccids_lock); > - > -/* > - * The strategy is: modifications ccids vector are short, do not sleep and > - * veeery rare, but read access should be free of any exclusive locks. > - */ > -static void ccids_write_lock(void) > -{ > - spin_lock(&ccids_lock); > - while (atomic_read(&ccids_lockct) != 0) { > - spin_unlock(&ccids_lock); > - yield(); > - spin_lock(&ccids_lock); > - } > -} > - > -static inline void ccids_write_unlock(void) > -{ > - spin_unlock(&ccids_lock); > -} > - > -static inline void ccids_read_lock(void) > -{ > - atomic_inc(&ccids_lockct); > - smp_mb__after_atomic_inc(); > - spin_unlock_wait(&ccids_lock); > -} > - > -static inline void ccids_read_unlock(void) > -{ > - atomic_dec(&ccids_lockct); > -} > - > -#else > -#define ccids_write_lock() do { } while(0) > -#define ccids_write_unlock() do { } while(0) > -#define ccids_read_lock() do { } while(0) > -#define ccids_read_unlock() do { } while(0) > -#endif > - > static struct kmem_cache *ccid_kmem_cache_create(int obj_size, const char *fmt,...) > { > struct kmem_cache *slab; > @@ -141,56 +98,33 @@ int ccid_getsockopt_builtin_ccids(struct > return 0; > } > > -int ccid_register(struct ccid_operations *ccid_ops) > +static int ccid_register(struct ccid_operations *ccid_ops) > { > - int err = -ENOBUFS; > - > ccid_ops->ccid_hc_rx_slab = > ccid_kmem_cache_create(ccid_ops->ccid_hc_rx_obj_size, > "ccid%u_hc_rx_sock", > ccid_ops->ccid_id); > if (ccid_ops->ccid_hc_rx_slab == NULL) > - goto out; > + return -ENOBUFS; You could have maintained the gotos, that way the patch would be smaller... > > ccid_ops->ccid_hc_tx_slab = > ccid_kmem_cache_create(ccid_ops->ccid_hc_tx_obj_size, > "ccid%u_hc_tx_sock", > ccid_ops->ccid_id); > - if (ccid_ops->ccid_hc_tx_slab == NULL) > - goto out_free_rx_slab; > > - ccids_write_lock(); I.e. we would see that in the end what you did here was just to remove the locking. > - err = -EEXIST; > - if (ccids[ccid_ops->ccid_id] == NULL) { > - ccids[ccid_ops->ccid_id] = ccid_ops; > - err = 0; > + if (ccid_ops->ccid_hc_tx_slab == NULL) { > + ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab); > + ccid_ops->ccid_hc_rx_slab = NULL; > + return -ENOBUFS; > } > - ccids_write_unlock(); > - if (err != 0) > - goto out_free_tx_slab; > > pr_info("CCID: Registered CCID %d (%s)\n", > ccid_ops->ccid_id, ccid_ops->ccid_name); > -out: > - return err; > -out_free_tx_slab: > - ccid_kmem_cache_destroy(ccid_ops->ccid_hc_tx_slab); > - ccid_ops->ccid_hc_tx_slab = NULL; > - goto out; > -out_free_rx_slab: > - ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab); > - ccid_ops->ccid_hc_rx_slab = NULL; > - goto out; > + return 0; > } > > -EXPORT_SYMBOL_GPL(ccid_register); > - > -int ccid_unregister(struct ccid_operations *ccid_ops) > +static int ccid_unregister(struct ccid_operations *ccid_ops) > { > - ccids_write_lock(); > - ccids[ccid_ops->ccid_id] = NULL; > - ccids_write_unlock(); > - > ccid_kmem_cache_destroy(ccid_ops->ccid_hc_tx_slab); > ccid_ops->ccid_hc_tx_slab = NULL; > ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab); And "register/unregister" now don't make much sense since there is no registration being done, just allocating/deallocating resources > @@ -201,14 +135,12 @@ int ccid_unregister(struct ccid_operatio > return 0; > } > > -EXPORT_SYMBOL_GPL(ccid_unregister); > - > int ccids_register_builtins(void) > { > int i, err; > > - for (i = 0; i < ARRAY_SIZE(builtin_ccids_ops); i++) { > - err = ccid_register(builtin_ccids_ops[i]); > + for (i = 0; i < ARRAY_SIZE(ccids); i++) { > + err = ccid_register(ccids[i]); > if (err) > goto unwind_registrations; > } > @@ -217,15 +149,31 @@ int ccids_register_builtins(void) > > unwind_registrations: > while(--i >= 0) > - ccid_unregister(builtin_ccids_ops[i]); > + ccid_unregister(ccids[i]); > return err; > } > > -static struct ccid *__ccid_new(struct ccid_operations *ccid_ops, struct sock *sk, > - int rx, gfp_t gfp) > + > +static struct ccid_operations *ccid_find_by_id(const u8 id) > { > - struct ccid *ccid = kmem_cache_alloc(rx ? ccid_ops->ccid_hc_rx_slab : > - ccid_ops->ccid_hc_tx_slab, gfp); > + int i; > + > + for (i = 0; i < ARRAY_SIZE(ccids); i++) > + if (ccids[i]->ccid_id == id) > + return ccids[i]; > + return NULL; Why the we searching? Can't we just do: { if (id > ARRAY_SIZE(ccids) - 2) return NULL; return ccids[id - 2]; } ? > +} > + > +struct ccid *ccid_new(const u8 id, struct sock *sk, bool rx) > +{ > + struct ccid_operations *ccid_ops = ccid_find_by_id(id); > + struct ccid *ccid; > + > + if (ccid_ops == NULL) > + return NULL; > + > + ccid = kmem_cache_alloc(rx ? ccid_ops->ccid_hc_rx_slab : > + ccid_ops->ccid_hc_tx_slab, gfp_any()); > if (ccid == NULL) > return NULL; > > @@ -241,58 +189,14 @@ static struct ccid *__ccid_new(struct cc > ccid->ccid_ops->ccid_hc_tx_init(ccid, sk) != 0) > goto out_free_ccid; > } > + > return ccid; > + > out_free_ccid: > kmem_cache_free(rx ? ccid_ops->ccid_hc_rx_slab : > ccid_ops->ccid_hc_tx_slab, ccid); > return NULL; > } > - > -static bool is_builtin_ccid(unsigned char id) > -{ > - int i; > - for (i = 0; i < ARRAY_SIZE(builtin_ccids); i++) > - if (id == builtin_ccids[i]) > - return true; > - return false; > -} > - > -struct ccid *ccid_new(unsigned char id, struct sock *sk, int rx, gfp_t gfp) > -{ > - struct ccid_operations *ccid_ops; > - struct ccid *ccid = NULL; > - > - if (is_builtin_ccid(id)) { > - ccid_ops = ccids[id]; > - BUG_ON(ccid_ops == NULL); > - return __ccid_new(ccid_ops, sk, rx, gfp); > - } > - > - ccids_read_lock(); > -#ifdef CONFIG_MODULES > - if (ccids[id] == NULL) { > - /* We only try to load if in process context */ > - ccids_read_unlock(); > - if (gfp & GFP_ATOMIC) > - goto out; > - request_module("net-dccp-ccid-%d", id); > - ccids_read_lock(); > - } > -#endif > - ccid_ops = ccids[id]; > - if (ccid_ops == NULL) > - goto out_unlock; > - > - ccids_read_unlock(); > - > - ccid = __ccid_new(ccid_ops, sk, rx, gfp); > -out: > - return ccid; > -out_unlock: > - ccids_read_unlock(); > - goto out; > -} > - > EXPORT_SYMBOL_GPL(ccid_new); > > static void ccid_delete(struct ccid *ccid, struct sock *sk, int rx) > --- a/net/dccp/feat.c > +++ b/net/dccp/feat.c > @@ -34,7 +34,7 @@ > static int dccp_hdlr_ccid(struct sock *sk, u64 ccid, bool rx) > { > struct dccp_sock *dp = dccp_sk(sk); > - struct ccid *new_ccid = ccid_new(ccid, sk, rx, gfp_any()); > + struct ccid *new_ccid = ccid_new(ccid, sk, rx); > > if (new_ccid == NULL) > return -ENOMEM; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
| > -int ccid_unregister(struct ccid_operations *ccid_ops)
| > +static int ccid_unregister(struct ccid_operations *ccid_ops)
| > {
<snip>
|
| And "register/unregister" now don't make much sense since there is no
| registration being done, just allocating/deallocating resources
|
Yes agree - this needs revision. We can also drop the EXPORT_SYMBOLS
from the now-static routines.
| > +
| > +static struct ccid_operations *ccid_find_by_id(const u8 id)
| > {
| > - struct ccid *ccid = kmem_cache_alloc(rx ? ccid_ops->ccid_hc_rx_slab :
| > - ccid_ops->ccid_hc_tx_slab, gfp);
| > + int i;
| > +
| > + for (i = 0; i < ARRAY_SIZE(ccids); i++)
| > + if (ccids[i]->ccid_id == id)
| > + return ccids[i];
| > + return NULL;
|
| Why the we searching? Can't we just do:
|
| {
| if (id > ARRAY_SIZE(ccids) - 2)
| return NULL;
| return ccids[id - 2];
| }
|
Agree 100%, I don't like the routine myself and have been thinking about
how to rewrite it. Current idea is to go back to the original and use
static struct ccid_operations *ccids[CCIDS_MAX] = {
[DCCPC_CCID2] = &ccid2_ops,
#ifdef CONFIG_IP_DCCP_CCID3
[DCCPC_CCID3] = &ccid3_ops,
#endif
};
And then use
if (id < 0 || id >= CCIDS_MAX)
return NULL;
return ccids[id];
which may be NULL if there is no entry in the array. Better?
Gerrit
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
| > -int ccid_unregister(struct ccid_operations *ccid_ops)
| > +static int ccid_unregister(struct ccid_operations *ccid_ops)
| > {
<snip>
|
| And "register/unregister" now don't make much sense since there is no
| registration being done, just allocating/deallocating resources
|
Yes agree - this needs revision. We can also drop the EXPORT_SYMBOLS
from the now-static routines.
| > +
| > +static struct ccid_operations *ccid_find_by_id(const u8 id)
| > {
| > - struct ccid *ccid = kmem_cache_alloc(rx ? ccid_ops->ccid_hc_rx_slab :
| > - ccid_ops->ccid_hc_tx_slab, gfp);
| > + int i;
| > +
| > + for (i = 0; i < ARRAY_SIZE(ccids); i++)
| > + if (ccids[i]->ccid_id == id)
| > + return ccids[i];
| > + return NULL;
|
| Why the we searching? Can't we just do:
|
| {
| if (id > ARRAY_SIZE(ccids) - 2)
| return NULL;
| return ccids[id - 2];
| }
|
Agree 100%, I don't like the routine myself and have been thinking about
how to rewrite it. Current idea is to go back to the original and use
static struct ccid_operations *ccids[CCIDS_MAX] = {
[DCCPC_CCID2] = &ccid2_ops,
#ifdef CONFIG_IP_DCCP_CCID3
[DCCPC_CCID3] = &ccid3_ops,
#endif
};
And then use
if (id < 0 || id >= CCIDS_MAX)
return NULL;
return ccids[id];
which may be NULL if there is no entry in the array. Better?
Gerrit
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
| | > + for (i = 0; i < ARRAY_SIZE(ccids); i++)
| | > + if (ccids[i]->ccid_id == id)
| | > + return ccids[i];
| | > + return NULL;
| |
| | Why the we searching? Can't we just do:
| |
| | {
| | if (id > ARRAY_SIZE(ccids) - 2)
| | return NULL;
| | return ccids[id - 2];
| | }
| |
| Agree 100%, I don't like the routine myself and have been thinking about
| how to rewrite it. Current idea is to go back to the original and use
|
| static struct ccid_operations *ccids[CCIDS_MAX] = {
| [DCCPC_CCID2] = &ccid2_ops,
| #ifdef CONFIG_IP_DCCP_CCID3
| [DCCPC_CCID3] = &ccid3_ops,
| #endif
| };
|
Unfortunately I found later that this solution introduces a lot of waste:
during initialisation, the code must step over 255 - 2 = 253 NULL entries
and the same happens when ejecting the CCID.
I went through several revisions and summarize the findings below.
The following functions need to query for built-in CCIDs:
bool ccid_support_check(u8 const *ccid_array, u8 array_len);
This is used by the feature-negotiation code to determine whether
all elements in 'ccid_array' are supported; for checking the CCID
setsockopt values and to check whether the CCID suggested by the
peer is locally available.
int ccid_get_builtin_ccids(u8 **ccid_array, u8 *array_len);
This is used during initialisation of the feature-negotiation code,
to create a list of the CCID values that can be advertised.
int ccid_getsockopt_builtin_ccids(struct sock *sk, int len, ...);
Used to provide the DCCP_SOCKOPT_AVAILABLE_CCIDS getsockopt option
which passes a list of available CCIDs to userspace.
struct ccid *ccid_new(unsigned char id, struct sock *sk, int rx, ...);
This function is called by the CCID handler in feat.c to set the RX/TX
CCID after it has been negotiated with the peer.
The first 3 functions can be implemented by simply walking through the
array, the last one requires to map a CCID value (Table 5 in RFC 4340,
10) into an array index. In the above, CCID ID = array index but only
2 out of 256 possible IDs (0..255) are non-empty (less than 0.8%).
Furthermore, the CCIDs need not be in consecutive order. For example
CCID-248 .. CCID-254 are experimental (RFC 4340, 19.5), so we could
have an array like
struct ccid_operations *ccids[] = {
&ccid2_ops, &ccid3_ops, &ccid247_ops, &ccid254_ops
};
This gives the mapping 0 +-> 2, 1 +-> 3, 2 +-> 247, 3 +-> 254. The
ccid_new() function needs the inverse mapping, e.g. for CCID-247 it
needs to find the third array entry.
So we have the choice of
* O(1) access for ccid_new() when using an array with 256 entries,
but have to pay the price of more complex registration /
unregistration routines. In addition, the 'get_builtin_xxx'
routines need to make two passes instead of one, to determine
the number of non-NULL entries.
* O(n) linear search to map a CCID number into the corresponding
array index, but simpler implementation of several routines.
Since n is much smaller than 255, I think the latter is better here.
[patch is on the way, need to finish testing]
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dave, please find attached an updated and tested revision of the earlier patch set to enable lockless use of CCID plugins within DCCP. It combines the initial patch by Arnaldo with further work, following discussions on this mailing list. All patches have been compile individually (sparse enabled) and have been verified to work. Wishing a good new year Gerrit Changes relative to earlier revision: ------------------------------------- * incorporated Arnaldo's feedback regarding the deletion of jump labels * and the renaming of the register/unregister functions (for want of a better word, 'activate' has been used here); * added un-registration function to allow module unloading; * removed now obsolute module references from the CCIDs (ccid_owner); * updated Kconfig menu to reflect all the new changes. List of patches: ---------------- Patch #1: Integrates CCID plugins with dccp.ko main module. Patch #2: Cleans up the old interface. Patch #3: Integrates the TFRC library, a dependency of CCID-3. The set is also available for online viewing, at http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=commitdiff;h=07b69e62ebcb01c4ec9e372e70aabbdd21d9c4fc CCID-4 has also been updated and likewise been tested - negotiation, as well as a longer test involving audio streaming. http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=shortlog;h=ccid4 Patch stats: ------------ net/dccp/Kconfig | 4 net/dccp/Makefile | 15 + net/dccp/ackvec.h | 53 ------ net/dccp/ccid.c | 300 ++++++++++++++---------------------- net/dccp/ccid.h | 14 - net/dccp/ccids/Kconfig | 73 ++------ net/dccp/ccids/Makefile | 9 - net/dccp/ccids/ccid2.c | 22 -- net/dccp/ccids/ccid3.c | 23 -- net/dccp/ccids/lib/Makefile | 3 net/dccp/ccids/lib/loss_interval.c | 3 net/dccp/ccids/lib/packet_history.c | 9 - net/dccp/ccids/lib/tfrc.c | 19 -- net/dccp/ccids/lib/tfrc.h | 11 + net/dccp/ccids/lib/tfrc_equation.c | 4 net/dccp/dccp.h | 2 net/dccp/feat.c | 6 net/dccp/input.c | 2 net/dccp/proto.c | 7 19 files changed, 186 insertions(+), 393 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/net/dccp/ccid.h +++ b/net/dccp/ccid.h @@ -19,14 +19,12 @@ #include <linux/list.h> #include <linux/module.h> -#define CCID_MAX 255 - struct tcp_info; /** * struct ccid_operations - Interface to Congestion-Control Infrastructure * - * @ccid_id: numerical CCID ID (up to %CCID_MAX, cf. table 5 in RFC 4340, 10.) + * @ccid_id: numerical CCID ID (cf. table 5 in RFC 4340, 10.) * @ccid_ccmps: the CCMPS including network/transport headers (0 when disabled) * @ccid_name: alphabetical identifier string for @ccid_id * @ccid_hc_{r,t}x_slab: memory pool for the receiver/sender half-connection @@ -93,9 +91,6 @@ extern struct ccid_operations ccid2_ops; extern struct ccid_operations ccid3_ops; #endif -extern int ccid_register(struct ccid_operations *ccid_ops); -extern int ccid_unregister(struct ccid_operations *ccid_ops); - extern int ccids_register_builtins(void); struct ccid { @@ -113,8 +108,7 @@ extern int ccid_get_builtin_ccids(u8 ** extern int ccid_getsockopt_builtin_ccids(struct sock *sk, int len, char __user *, int __user *); -extern struct ccid *ccid_new(unsigned char id, struct sock *sk, int rx, - gfp_t gfp); +extern struct ccid *ccid_new(const u8 id, struct sock *sk, bool rx); static inline int ccid_get_current_rx_ccid(struct dccp_sock *dp) { --- a/net/dccp/ccid.c +++ b/net/dccp/ccid.c @@ -13,8 +13,8 @@ #include "ccid.h" -static struct ccid_operations *builtin_ccids_ops[] = { - &ccid2_ops, /* CCID2 is supported by default */ +static struct ccid_operations *ccids[] = { + &ccid2_ops, /* CCID-2 is supported by default */ #ifdef CONFIG_IP_DCCP_CCID3 &ccid3_ops, #endif @@ -27,49 +27,6 @@ static u8 builtin_ccids[] = { #endif }; -static struct ccid_operations *ccids[CCID_MAX]; -#if defined(CONFIG_SMP) || defined(CONFIG_PREEMPT) -static atomic_t ccids_lockct = ATOMIC_INIT(0); -static DEFINE_SPINLOCK(ccids_lock); - -/* - * The strategy is: modifications ccids vector are short, do not sleep and - * veeery rare, but read access should be free of any exclusive locks. - */ -static void ccids_write_lock(void) -{ - spin_lock(&ccids_lock); - while (atomic_read(&ccids_lockct) != 0) { - spin_unlock(&ccids_lock); - yield(); - spin_lock(&ccids_lock); - } -} - -static inline void ccids_write_unlock(void) -{ - spin_unlock(&ccids_lock); -} - -static inline void ccids_read_lock(void) -{ - atomic_inc(&ccids_lockct); - smp_mb__after_atomic_inc(); - spin_unlock_wait(&ccids_lock); -} - -static inline void ccids_read_unlock(void) -{ - atomic_dec(&ccids_lockct); -} - -#else -#define ccids_write_lock() do { } while(0) -#define ccids_write_unlock() do { } while(0) -#define ccids_read_lock() do { } while(0) -#define ccids_read_unlock() do { } while(0) -#endif - static struct kmem_cache *ccid_kmem_cache_create(int obj_size, const char *fmt,...) { struct kmem_cache *slab; @@ -141,56 +98,33 @@ int ccid_getsockopt_builtin_ccids(struct return 0; } -int ccid_register(struct ccid_operations *ccid_ops) +static int ccid_register(struct ccid_operations *ccid_ops) { - int err = -ENOBUFS; - ccid_ops->ccid_hc_rx_slab = ccid_kmem_cache_create(ccid_ops->ccid_hc_rx_obj_size, "ccid%u_hc_rx_sock", ccid_ops->ccid_id); if (ccid_ops->ccid_hc_rx_slab == NULL) - goto out; + return -ENOBUFS; ccid_ops->ccid_hc_tx_slab = ccid_kmem_cache_create(ccid_ops->ccid_hc_tx_obj_size, "ccid%u_hc_tx_sock", ccid_ops->ccid_id); - if (ccid_ops->ccid_hc_tx_slab == NULL) - goto out_free_rx_slab; - ccids_write_lock(); - err = -EEXIST; - if (ccids[ccid_ops->ccid_id] == NULL) { - ccids[ccid_ops->ccid_id] = ccid_ops; - err = 0; + if (ccid_ops->ccid_hc_tx_slab == NULL) { + ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab); + ccid_ops->ccid_hc_rx_slab = NULL; + return -ENOBUFS; } - ccids_write_unlock(); - if (err != 0) - goto out_free_tx_slab; pr_info("CCID: Registered CCID %d (%s)\n", ccid_ops->ccid_id, ccid_ops->ccid_name); -out: - return err; -out_free_tx_slab: - ccid_kmem_cache_destroy(ccid_ops->ccid_hc_tx_slab); - ccid_ops->ccid_hc_tx_slab = NULL; - goto out; -out_free_rx_slab: - ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab); - ccid_ops->ccid_hc_rx_slab = NULL; - goto out; + return 0; } -EXPORT_SYMBOL_GPL(ccid_register); - -int ccid_unregister(struct ccid_operations *ccid_ops) +static int ccid_unregister(struct ccid_operations *ccid_ops) { - ccids_write_lock(); - ccids[ccid_ops->ccid_id] = NULL; - ccids_write_unlock(); - ccid_kmem_cache_destroy(ccid_ops->ccid_hc_tx_slab); ccid_ops->ccid_hc_tx_slab = NULL; ccid_kmem_cache_destroy(ccid_ops->ccid_hc_rx_slab); @@ -201,14 +135,12 @@ int ccid_unregister(struct ccid_operatio return 0; } -EXPORT_SYMBOL_GPL(ccid_unregister); - int ccids_register_builtins(void) { int i, err; - for (i = 0; i < ARRAY_SIZE(builtin_ccids_ops); i++) { - err = ccid_register(builtin_ccids_ops[i]); + for (i = 0; i < ARRAY_SIZE(ccids); i++) { + err = ccid_register(ccids[i]); if (err) goto unwind_registrations; } @@ -217,15 +149,31 @@ int ccids_register_builtins(void) unwind_registrations: while(--i >= 0) - ccid_unregister(builtin_ccids_ops[i]); + ccid_unregister(ccids[i]); return err; } -static struct ccid *__ccid_new(struct ccid_operations *ccid_ops, struct sock *sk, - int rx, gfp_t gfp) + +static struct ccid_operations *ccid_find_by_id(const u8 id) { - struct ccid *ccid = kmem_cache_alloc(rx ? ccid_ops->ccid_hc_rx_slab : - ccid_ops->ccid_hc_tx_slab, gfp); + int i; + + for (i = 0; i < ARRAY_SIZE(ccids); i++) + if (ccids[i]->ccid_id == id) + return ccids[i]; + return NULL; +} + +struct ccid *ccid_new(const u8 id, struct sock *sk, bool rx) +{ + struct ccid_operations *ccid_ops = ccid_find_by_id(id); + struct ccid *ccid; + + if (ccid_ops == NULL) + return NULL; + + ccid = kmem_cache_alloc(rx ? ccid_ops->ccid_hc_rx_slab : + ccid_ops->ccid_hc_tx_slab, gfp_any()); if (ccid == NULL) return NULL; @@ -241,58 +189,14 @@ static struct ccid *__ccid_new(struct cc ccid->ccid_ops->ccid_hc_tx_init(ccid, sk) != 0) goto out_free_ccid; } + return ccid; + out_free_ccid: kmem_cache_free(rx ? ccid_ops->ccid_hc_rx_slab : ccid_ops->ccid_hc_tx_slab, ccid); return NULL; } - -static bool is_builtin_ccid(unsigned char id) -{ - int i; - for (i = 0; i < ARRAY_SIZE(builtin_ccids); i++) - if (id == builtin_ccids[i]) - return true; - return false; -} - -struct ccid *ccid_new(unsigned char id, struct sock *sk, int rx, gfp_t gfp) -{ - struct ccid_operations *ccid_ops; - struct ccid *ccid = NULL; - - if (is_builtin_ccid(id)) { - ccid_ops = ccids[id]; - BUG_ON(ccid_ops == NULL); - return __ccid_new(ccid_ops, sk, rx, gfp); - } - - ccids_read_lock(); -#ifdef CONFIG_MODULES - if (ccids[id] == NULL) { - /* We only try to load if in process context */ - ccids_read_unlock(); - if (gfp & GFP_ATOMIC) - goto out; - request_module("net-dccp-ccid-%d", id); - ccids_read_lock(); - } -#endif - ccid_ops = ccids[id]; - if (ccid_ops == NULL) - goto out_unlock; - - ccids_read_unlock(); - - ccid = __ccid_new(ccid_ops, sk, rx, gfp); -out: - return ccid; -out_unlock: - ccids_read_unlock(); - goto out; -} - EXPORT_SYMBOL_GPL(ccid_new); static void ccid_delete(struct ccid *ccid, struct sock *sk, int rx) --- a/net/dccp/feat.c +++ b/net/dccp/feat.c @@ -34,7 +34,7 @@ static int dccp_hdlr_ccid(struct sock *sk, u64 ccid, bool rx) { struct dccp_sock *dp = dccp_sk(sk); - struct ccid *new_ccid = ccid_new(ccid, sk, rx, gfp_any()); + struct ccid *new_ccid = ccid_new(ccid, sk, rx); if (new_ccid == NULL) return -ENOMEM;
dccp: Lockless use of CCIDs This updates the implementation to use only a single array whose size equals the number of configured CCIDs instead of 255. Since the CCIDs are fixed array elements, synchronization is no longer needed. Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk> --- net/dccp/ccid.h | 10 --- net/dccp/ccid.c | 166 +++++++++++--------------------------------------------- net/dccp/feat.c | 2 3 files changed, 38 insertions(+), 140 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html