(SPAM?) [openib-general] [PATCH] ping Add IB ping server agent

[Sean Hefty]

Hal Rosenstock wrote:
> +	ping_send_wr = kmalloc(sizeof(*ping_send_wr), GFP_KERNEL);
> +	if (!ping_send_wr)
> +		goto out;
> +	ping_send_wr->mad = mad_priv;
> +
> +	/* PCI mapping */
> +	gather_list.addr = dma_map_single(mad_agent->device->dma_device,
> +					  &mad_priv->mad,
> +					  sizeof(mad_priv->mad),
> +					  DMA_TO_DEVICE);
> +	gather_list.length = sizeof(mad_priv->mad);
> +	gather_list.lkey = mad_agent->mr->lkey;
> +
> +	send_wr.next = NULL;
> +	send_wr.opcode = IB_WR_SEND;
> +	send_wr.sg_list = &gather_list;
> +	send_wr.num_sge = 1;
> +	send_wr.wr.ud.remote_qpn = wc->src_qp; /* DQPN */
> +	send_wr.wr.ud.timeout_ms = 0;
> +	send_wr.send_flags = IB_SEND_SIGNALED | IB_SEND_SOLICITED;
> +
> +	ah_attr.dlid = wc->slid;
> +	ah_attr.port_num = mad_agent->port_num;
> +	ah_attr.src_path_bits = wc->dlid_path_bits;
> +	ah_attr.sl = wc->sl;
> +	ah_attr.static_rate = 0;
> +	ah_attr.ah_flags = 0; /* No GRH */
> +	if (mad_priv->mad.mad.mad_hdr.mgmt_class == IB_MGMT_CLASS_OPENIB_PING) {
> +		if (wc->wc_flags & IB_WC_GRH) {
> +			ah_attr.ah_flags = IB_AH_GRH;
> +			/* Should sgid be looked up ? */
> +			ah_attr.grh.sgid_index = 0;
> +			ah_attr.grh.hop_limit = grh->hop_limit;
> +			ah_attr.grh.flow_label = be32_to_cpup(
> +				&grh->version_tclass_flow)  & 0xfffff;
> +			ah_attr.grh.traffic_class = (be32_to_cpup(
> +				&grh->version_tclass_flow) >> 20) & 0xff;
> +			memcpy(ah_attr.grh.dgid.raw,
> +			       grh->sgid.raw,
> +			       sizeof(ah_attr.grh.dgid));
> +		}

We should start looking at moving code like this into a common function 
usable by multiple modules.  This would require exposing the definition 
of some sort of send_mad structure, but I think that we can come up 
with something that would work for most agents.

> +		kmem_cache_free(ib_mad_cache, recv);

Why doesn't this code just call ib_free_recv_mad() like other agents 
do?  This assumes the implementation of the MAD layer, which I think 
can be avoided.

> +	/* Unmap PCI */
> +	dma_unmap_single(mad_agent->device->dma_device,
> +			 pci_unmap_addr(ping_send_wr, mapping),
> +			 sizeof(ping_send_wr->mad->mad),
> +			 DMA_TO_DEVICE);
> +
> +	ib_destroy_ah(ping_send_wr->ah);
> +
> +	/* Release allocated memory */
> +	kmem_cache_free(ib_mad_cache, ping_send_wr->mad);
> +	kfree(ping_send_wr);

I don't like that ib_mad_cache is being used for send MADs, when it 
stores and is used for MADs posted to the receive queue.  The use of 
this cache by agents makes it more difficult to change the MAD layer 
implementation.

> Index: mad.c
> ===================================================================
> --- mad.c	(revision 2023)
> +++ mad.c	(working copy)
> @@ -45,6 +45,8 @@
>  
>  
>  kmem_cache_t *ib_mad_cache;
> +EXPORT_SYMBOL(ib_mad_cache);
> +

I'm not sure about exporting the cache in this manner, versus providing 
additional functionality.  What I think makes sense to do is to examine 
the SA query, CM, ping, and RMPP code to identify areas of re-use for 
sending MADs.

This may get us back to providing a virtual pool of send MADs, which 
could later be combined with the received MADs that would allow turning 
a receive MAD around as a response.

- Sean