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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
