| > | > +static int ccid_request_module(u8 id)

| > | > +{

| > | > +       if (!in_atomic()) {

| > | > +               ccids_read_lock();

| > | > +               if (ccids[id] == NULL) {

| > | > +                       ccids_read_unlock();

| > | > +                       return request_module("net-dccp-ccid-%d", id);

| > | > +               }

| > | > +               ccids_read_unlock();

| > | > +       }

| > | > +       return 0;

| > | > +}

| > |

| > | Just a random thought: does this lock really do anything useful here?

| > |

| > Reading the (shared) 'ccids' array is the solution chosen to check whether

| > the module for CCID with number 'id' is already loaded.

| >

| > It would be bad to call request_module() each time a new DCCP socket is

| > opened. Using the 'ccids' array may not be the only way to check whether

| > a given module (whose name depends on the value of 'id') is loaded.

| >

| > But if this solution is chosen, then it requires to protect the read

| > access to 'ccids', which is shared among all DCCP sockets.

|

| Since the lock is dropped after checking ccids[id] then there's

| a window where multiple request_module()s can be called if multiple

| applications create a DCCP socket at a same time. The code below

| should do the same without a lock (ccids is a static array,

| so ccids[N] is always at the same place).

|

| static int ccid_request_module(u8 id)

| {

|        if (!in_atomic()) {

|                rmb();

|                if (ccids[id] == NULL)

|                        return request_module("net-dccp-ccid-%d", id);

|        }

|        return 0;

| }

|

Sorry Michael, but this is really just a "random thought". What you are

in effect saying is that reader/writer locks can be replaced with just a

read memory barrier.
Please have a more detailed look at net/dccp/ccid.c. I also checked how

other subsystems handle comparable situations of module loading: the

implementation details differ, but the principle is the same: there are

mutexes, semaphores, and spinlocks in use to protect those shared

structures that are related to the loaded module.
Hence your suggestion does not improve the code. I maintain that it is

correct. And it has proven to work in the test tree for more than one

year, including tests with up to 100 parallel (iperf) connections.

--

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