[openib-general] [RFC] [PATCH] Optimize access to method->agent using bitops

[Krishna Kumar]

Hi Hal,

I didn't fix the argument to this routine, I was trying to understand if
the idea behind this will work, hence the RFC in the subject. Sorry for
creating the confusion. I was trying to understand if what I was assuming
is right or not.

The first part of the patch checks if any bit is set in the method_mask,
and if so, it means that a method (table?) is registered and hence it
returns error. Actually there might be a better way to check if the
bitmask is all-zero's and avoid the for loop, but I don't see any macros
for that, and I didn't want to use "if (method_mask)".

The add_mad_reg_req() code is doing :
	/* Finally, add in methods being registered */
        for (i = find_first_bit(mad_reg_req->method_mask,IB_MGMT_MAX_METHODS);
	     i < IB_MGMT_MAX_METHODS;
	     i = find_next_bit(mad_reg_req->method_mask, IB_MGMT_MAX_METHODS,
                               1+i)) {
                (*method)->agent[i] = priv;
        }
So the agent[0-128] is pointing to the priv when that particular bitmask
is set in the method_mask (exact same bit number is used as index in
agent).

Do you think this model is correct ? The Get/Set/Repress functions can be
checked faster by checking if the first/next bit being set rather than
going through the entire array of 128 agents, or in one case whether the
bitmask is zero or non-zero instead of looping 128 times. If this is true,
I will recreate the patch and compile before sending in the final patch.

thx,

- KK

On Tue, 2 Nov 2004, Hal Rosenstock wrote:

> On Tue, 2004-11-02 at 15:09, Krishna Kumar wrote:
> > I am not entirely sure that I understand the bitwise operator being
> > used in the code. Following patch is assuming that I have got it
> > right :-).
> >
> > thanks,
> >
> > - KK
> >
> > diff -ruNp 5/mad.c 6/mad.c
> > --- 5/mad.c	2004-11-02 12:07:51.000000000 -0800
> > +++ 6/mad.c	2004-11-02 12:08:32.000000000 -0800
> > @@ -537,9 +537,13 @@ static int check_method_table(struct ib_
> >  {
> >  	int i;
> >
> > -	for (i = 0; i < IB_MGMT_MAX_METHODS; i++)
> > -		if (method->agent[i])
> > -			return 1;
> > +	for (i = find_first_bit(mad_reg_req->method_mask, IB_MGMT_MAX_METHODS);
> > +	     i < IB_MGMT_MAX_METHODS;
> > +	     i = find_next_bit(mad_reg_req->method_mask, IB_MGMT_MAX_METHODS,
> > +			       1+i)) {
> > +		/* if we entered the loop, we have found an agent bit set */
> > +		return 1;
> > +	}
> >  	return 0;
> >  }
>
> This is no longer checking the method table. It is checking the
> registration request. Also, a pointer to the registration request
> would need to be passed into this routine if it is to be used.
>
> > @@ -561,11 +565,13 @@ static void remove_methods_mad_agent(str
> >  {
> >  	int i;
> >
> > -	/* Remove any methods for this mad agent */
> > -	for (i = 0; i < IB_MGMT_MAX_METHODS; i++) {
> > -		if (method->agent[i] == agent) {
> > -			method->agent[i] = NULL;
> > -		}
> > +	/* Remove all methods for this mad agent */
> > +	for (i = find_first_bit(mad_reg_req->method_mask, IB_MGMT_MAX_METHODS);
> > +	     i < IB_MGMT_MAX_METHODS;
> > +	     i = find_next_bit(mad_reg_req->method_mask, IB_MGMT_MAX_METHODS,
> > +			       1+i)) {
> > +		BUG_ON(method->agent[i] != agent);
> > +		method->agent[i] = NULL;
> >  	}
> >  }
>
> Same compilation issue as above:
> A pointer to the registration request would need to be passed into this
> routine if it is to be used.
>
> -- Hal
>
>
>