Re: [PATCHv3] opensm: Reduce heap consumption by multicast routing tables (MFTs)

Hal Rosenstock
2009-10-16T13:28:47Z

On Fri, Oct 16, 2009 at 8:24 AM, Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org> wrote:
> On 18:35 Thu 15 Oct     , Hal Rosenstock wrote:
>> >> @@ -138,6 +122,51 @@ void osm_mcast_tbl_set(IN osm_mcast_tbl_t * p_tbl, IN uint16_t mlid_ho,
>> >>
>> >>  /**********************************************************************
>> >>   **********************************************************************/
>> >> +void
>> >> +osm_mcast_tbl_realloc_mask_tbl(IN osm_mcast_tbl_t * p_tbl,
>> >> +                            IN osm_subn_t * p_subn, IN uintn_t mlid_offset)
>> >
>> > I would suggest to simplify the function name to something like
>> > 'osm_mcast_tbl_realloc()' - osm_mcast_tbl API doesn't have any
>> > information about a buffer internal structure.
>> >
>> >
>> >> +{
>> >> +     size_t mft_size, size;
>> >> +     uint16_t (*p_mask_tbl)[][IB_MCAST_POSITION_MAX];
>> >> +
>> >> +     if (mlid_offset < p_tbl->mft_size)
>> >> +             return;
>> >> +
>> >> +     /*
>> >> +        The number of bytes needed in the mask table is:
>> >> +        The (maximum bit mask 'position' + 1) times the
>> >> +        number of bytes in each bit mask times the
>> >> +        number of MLIDs supported by the table.
>> >> +
>> >> +        We must always allocate the array with the maximum position
>> >> +        since it is (and must be) defined that way the table structure
>> >> +        in order to create a pointer to a two dimensional array.
>> >> +      */
>> >> +     mft_size = (mlid_offset + IB_MCAST_BLOCK_SIZE) /
>> >> +                 IB_MCAST_BLOCK_SIZE * IB_MCAST_BLOCK_SIZE;
>> >> +     if (mft_size > (p_tbl->max_block + 1) * IB_MCAST_BLOCK_SIZE)
>> >> +             mft_size = (p_tbl->max_block + 1) * IB_MCAST_BLOCK_SIZE;
>> >
>> > Hmm, wouldn't this:
>> >
>> >        mft_size = (mlid_offset / IB_MCAST_BLOCK_SIZE + 1) * IB_MCAST_BLOCK_SIZE;
>> >
>> > do the same as lines above?
>>
>> What about the limit (max_block) check ?
>
> If passed mlid_offset can exceed this then it is likely needed, but I'm
> not sure that this is the case - then we are asking for more mlids than
> a subnet is capable to handle and probably it is blocked somewhere
> already (but I didn't check really).

You're right; this check is not needed.

>
>> >> @@ -154,6 +183,8 @@ boolean_t osm_mcast_tbl_is_port(IN const osm_mcast_tbl_t * p_tbl,
>> >>               CL_ASSERT(mlid_ho <= p_tbl->max_mlid_ho);
>> >>
>> >>               mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
>> >> +             if (mlid_offset >= p_tbl->mft_size)
>> >> +                     return FALSE;
>> >
>> > If you are introducing new field mft_size, I would also suggest to change
>> > the meaning of max_mlid_ho field to be max configured mlid for this
>> > table and not max capability mlid (which is almost duplicated by
>> > num_entries field).
>>
>> Yes, almost duplicated.
>>
>> > I suppose that this can be done as separate patch.
>> >
>> > Then all such and similar (many below) checks should be performed against
>> > this actually configured max mlid and not against table size. As we
>> > discussed already this prevents some bugs for 'max_mlid < table_size - 1'
>> > case.
>>
>> I'll look at this subsequent to this patch.
>
> I would suggest to make it first (guess that it is possible without
> relation to MFT reallocation), so we will need to change all those
> lines only once.

OK.

-- Hal

> Sasha