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

[Sasha Khapyorsky <sashak-smomgflXvOZWk0Htik3J/w@public.gmane.org>]

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).

> >> @@ -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.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html