On Thu, 2004-10-14 at 13:55, Sean Hefty wrote:
> I'm not quite understanding this change. If the user has provided a mad_reg_req,
> they are indicating that they want to receive unsolicited MADs. A recv_handler
> should be required.
> Am I missing something?
What if they didn't fill in any methods in their registration request ?
Should a recv_handler be required ? Note that the mthca appears to be a
send only MAD client (for locally generated traps). That's what all this
stemmed from. Maybe I took it too far.
> If I understand this change, a client sent a MAD, expecting a response, got one,
> but didn't register with a receive handler.
Maybe he sent something and wasn't expecting a response (so didn't
register a recv handler) but got one anyway. Should the MAD layer crash
because of this ?
> As a side thought, I'm wondering how much protection we need to build into the code
> to
> handle kernel clients that don't provide all of the necessary parameters, but we can
> discuss this.
I've been wondering this myself too. In the previous case, it caused the
MAD layer to crash on a NULL pointer reference and the Linux locked up
sometime thereafter.
> The only real problem with this change is that the receive buffer needs to be
> released
> if it is not given to a client.
I think it's there at the end of the ib_mad_recv_done_handler where
!mad_agent->agent.recv_handler is checked and kmem_cache_free is called
if so. I see the problem you have with doing it this way later in your
email so I will fix it.
> We should probably change the send status as well, since no response was delivered.
OK. What status would you propose ? Do you want to generate a patch for
this or should I ?
> > @@ -851,7 +865,9 @@
> > mad_send_wc.wr_id = mad_send_wr->wr_id;
> > ib_mad_complete_send_wr(mad_send_wr, &mad_send_wc);
> > } else {
> > - mad_agent_priv->agent.recv_handler(&mad_agent_priv->agent,
> > + if (mad_agent_priv->agent.recv_handler)
> > + mad_agent_priv->agent.recv_handler(
> > + &mad_agent_priv->agent,
> > &recv->header.recv_wc);
>
> Need to free the receive buffer here as well if not delivered.
This was to be handled by the code at the end of
ib_mad_recv_done_handler. I will fix it.
> > - if (!mad_agent) {
> > + if (!mad_agent || !mad_agent->agent.recv_handler) {
>
> This appears to be where the receive buffer would have been freed, but...
> We can't safely walk into the mad_agent structure after calling
> ib_mad_complete_recv().
> Immediately above this code a reference is taken on the mad_agent.
> That reference is released in ib_mad_complete_recv(), which would allow the user to
> destroy the mad_agent before returning back to this call and the if statement above.
OK. I will move the buffer releases to where the references are held.
> I'm more in favor of removing checks for a recv_handler completely,
> but if we want to keep it, we can move it into find_mad_agent(),
> and just not report a mad_agent if it doesn't have a recv_handler.
That's a much better solution. As to the checks for the recv_handler, it
depends on whether a recv_handler is required. Is it is, a simple check
during registration and removal of the checks for whether than recv
handler was supplied (the way it was). I thought it more flexible to
make this optional but only got part way there with the implementation.
> > if (mad_send_wr->status != IB_WC_SUCCESS )
> > mad_send_wc->status = mad_send_wr->status;
> > - mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> > mad_send_wc);
> > + if (mad_agent_priv->agent.send_handler)
> > + mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> > + mad_send_wc);
>
> This has similar problems to the receive handling. If a client issued a send,
> but doesn't have a send_handler, there's nothing we can do with the send buffer,
> which needs to be freed. I think that a client who does this is causing more
> problems then we can deal with in the access layer.
Agreed. So are send handlers always required ? I just continued with
thinking that if receive handlers are optional, might send ones be too
in certain cases ?
> A possible fix for this is to check that mad_agent has a send_handler when the send
> is posted,
> rather than waiting until it completes.
Yes, that's a better solution.
> > /* Release reference on agent taken when sending */
> > if (atomic_dec_and_test(&mad_agent_priv->refcount))
> > @@ -1135,7 +1153,9 @@
> > list_for_each_entry_safe(mad_send_wr, temp_mad_send_wr,
> > &cancel_list, agent_list) {
> > mad_send_wc.wr_id = mad_send_wr->wr_id;
> > - mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> > + if (mad_agent_priv->agent.send_handler)
> > + mad_agent_priv->agent.send_handler(
> > + &mad_agent_priv->agent,
> > &mad_send_wc);
>
> Same issue as above.
>
> > list_del(&mad_send_wr->agent_list);
> > @@ -1196,8 +1216,9 @@
> > mad_send_wc.status = IB_WC_WR_FLUSH_ERR;
> > mad_send_wc.vendor_err = 0;
> > mad_send_wc.wr_id = mad_send_wr->wr_id;
> > - mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> > - &mad_send_wc);
> > + if (mad_agent_priv->agent.send_handler)
> > + mad_agent_priv->agent.send_handler(&mad_agent_priv->agent,
> > + &mad_send_wc);
>
> Ditto.
-- Hal
_______________________________________________
openib-general mailing list
[EMAIL PROTECTED]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
