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

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

Hi Hal,

On 09:47 Fri 30 Oct     , Hal Rosenstock wrote:
> >> @@ -152,10 +166,11 @@ boolean_t osm_mcast_tbl_is_port(IN const osm_mcast_tbl_t * p_tbl,
> >>               CL_ASSERT(port_num <=
> >>                         (p_tbl->max_position + 1) * IB_MCAST_MASK_SIZE);
> >>               CL_ASSERT(mlid_ho >= IB_LID_MCAST_START_HO);
> >> -             CL_ASSERT(mlid_ho <= (uint16_t) (IB_LID_MCAST_START_HO +
> >> -                                              p_tbl->num_entries - 1));
> >> +             CL_ASSERT(mlid_ho <= p_tbl->max_mlid_ho);
> >>
> >>               mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
> >> +             if (mlid_offset >= p_tbl->mft_depth)
> >> +                     return FALSE;
> >
> > This duplicates CL_ASSERT() above.
> 
> Not quite. There's a difference between max_mlid_ho and mft_depth.

And 'max_mlid_ho <= mft_depth + IB_LID_MCAST_START_HO - 1', isn't it?

> > Looking on how this function is used
> > I don't see why we this check should be introduced. Do you?
> 
> It's needed for group removal to work properly.

Could you elaborate? osm_mcast_tbl_is_port() is called only from
osm_switch_recommend_mcast_path(), how is it related to group removal?

> 
> 
> >>               mask_offset = port_num / IB_MCAST_MASK_SIZE;
> >>               bit_mask = cl_ntoh16((uint16_t)
> >>                                    (1 << (port_num % IB_MCAST_MASK_SIZE)));
> >> @@ -180,10 +195,11 @@ boolean_t osm_mcast_tbl_is_any_port(IN const osm_mcast_tbl_t * p_tbl,
> >>
> >>       if (p_tbl->p_mask_tbl) {
> >>               CL_ASSERT(mlid_ho >= IB_LID_MCAST_START_HO);
> >> -             CL_ASSERT(mlid_ho <= (uint16_t) (IB_LID_MCAST_START_HO +
> >> -                                              p_tbl->num_entries - 1));
> >> +             CL_ASSERT(mlid_ho <= p_tbl->max_mlid_ho);
> >>
> >>               mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
> >> +             if (mlid_offset >= p_tbl->mft_depth)
> >> +                     return FALSE;
> >
> > Ditto.
> 
> See above.

And this function osm_mcast_tbl_is_any_port() is not called at all. Is
it?

> >> @@ -223,9 +238,6 @@ ib_api_status_t osm_mcast_tbl_set_block(IN osm_mcast_tbl_t * p_tbl,
> >>       if (block_num > p_tbl->max_block_in_use)
> >>               p_tbl->max_block_in_use = (uint16_t) block_num;
> >>
> >> -     if (mlid_start_ho + IB_MCAST_BLOCK_SIZE - 1 > p_tbl->max_mlid_ho)
> >> -             p_tbl->max_mlid_ho = mlid_start_ho + IB_MCAST_BLOCK_SIZE - 1;
> >> -
> >>       return IB_SUCCESS;
> >>  }
> >>
> >> @@ -241,6 +253,8 @@ void osm_mcast_tbl_clear_mlid(IN osm_mcast_tbl_t * p_tbl, IN uint16_t mlid_ho)
> >>
> >>       if (p_tbl->p_mask_tbl && (mlid_ho <= p_tbl->max_mlid_ho)) {
> >>               mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
> >> +             if (mlid_offset >= p_tbl->mft_depth)
> >> +                     return;
> >
> > This seems redundant for me after 'mlid_ho <= p_tbl->max_mlid_ho' check
> > above.
> 
> See above.

I see nothing related above. Again:

This is what we will have after your patch:

	if (p_tbl->p_mask_tbl && (mlid_ho <= p_tbl->max_mlid_ho)) {
		mlid_offset = mlid_ho - IB_LID_MCAST_START_HO;
		if (mlid_offset >= p_tbl->mft_depth)
			return;

, and assuming that:

	p_tbl->max_mlid_ho = p_tbl->mft_depth + IB_LID_MCAST_START_HO - 1

, how the second if () condition could be true?

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