[openib-general] [PATCH v2 1/11] Driver Main files - netdev functions and corresponding state maintenance

[Ramachandra Kuchimanchi]

Sorry for the very late reply. I am catching up on my communication.
Please see inline.

Bryan O'Sullivan wrote:
> Ramachandra K wrote:
> 
> > +void vnic_connected(struct vnic *vnic, struct netpath *netpath) {
> > +	VNIC_FUNCTION("vnic_connected()\n");
> > +	if (netpath->second_bias)
> > +		vnic_npevent_queue_evt(netpath, VNIC_SECNP_CONNECTED);
> > +	else
> > +		vnic_npevent_queue_evt(netpath, VNIC_PRINP_CONNECTED);
> > +
> > +	vnic_connected_stats(vnic);
> > +}
> 
> This driver looks rather like it's duplicating at least some of the
> functionality provided by the bonding driver.  Could you explain the
> differences between the two approaches, please?

I looked up the bonding driver approach and see that there is some amount
of overlap with the functionality of the bonding driver. I will be investigating
this further. But one difference that comes to mind immediately is that as
compared to the bonding driver, in this approach another interface need
not be created for failover to work. 

> 
> > +void vnic_stop_xmit(struct vnic *vnic, struct netpath *netpath) {
> > +	VNIC_FUNCTION("vnic_stop_xmit()\n");
> > +	if (netpath == vnic->current_path) {
> > +		if (vnic->xmit_started) {
> > +			netif_stop_queue(&vnic->netdevice);
> > +			vnic->xmit_started = 0;
> > +		}
> > +
> > +		vnic_stop_xmit_stats(vnic);
> > +	}
> > +}
> 
> Why is there an asymmetry between some of the routines, which will operate on
> either netpath depending on the bias setting, and others like this, which only
> operate on the current netpath and silently do nothing otherwise?  This is at
> least confusing.

The logic is that netdev entry point functions operate on the current path
whereas most the other functions (called from vnic_viport etc) queue events based on
the path. (This is explained in detail below). In the above function calling
netif_stop_queue() makes sense only if the netpath is the current path and
the transmit has started.

> 
> > +static struct vnic * vnic_handle_npevent(struct vnic *vnic,
> > +					 enum vnic_npevent_type npevt_type) {
> > +	struct netpath	*netpath;
> > +
> > +	VNIC_INFO("%s: processing %s, netpath=%s, carrier=%d\n",
> > +		  vnic->config->name, vnic_npevent_str[npevt_type],
> > +		  netpath_to_string(vnic, vnic->current_path),
> > +		  vnic->carrier);
> > +
> > +	switch (npevt_type) {
> > +	case VNIC_PRINP_CONNECTED:
> 
> I still don't understand this business of queueing events in the normal net
> driver entry points, then redispatching them in this big switch statement.
> Could you at least add a comment to the code that indicates why you're doing
> this?  This structure makes the driver harder to follow without providing any
> obvious benefits.

The queueing of events does not happen in the netdev entry point functions
(except for open and stop) but in the functions that are called from
outside vnic_main.c. The knowledge of primary and secondary paths and the
corresponding actions for these paths is encapsulated in vnic_main.c.
Other parts of the code simply operate on the netpath they are associated
with and call into vnic_main.c functions which queue the events for the
netpath state machine thread depending on the type of path.

For example, when control and data connections are successfully established with the VEx,
vnic_viport.c calls vnic_connected() with the netpath that is associated with the viport.
vnic_connected() then queues a VNIC_PRINP_CONNECTED or a VNIC_SECNP_CONNECTED event
depending on whether it’s the primary or the secondary path that connected. There are
two different events because the action that has to be taken is different when each path connects.
When the primary path connects, register_netdev() has to be called and when secondary path
connects, it just means that the failover path is available. 

I will add some comments in the code to make this more clear.

Regards,
Ram