Change in vdsm[master]: netlink: Introduce socket pool

danken at redhat.com danken at redhat.com
Wed Feb 19 11:43:40 UTC 2014


Dan Kenigsberg has posted comments on this change.

Change subject: netlink: Introduce socket pool
......................................................................


Patch Set 3:

(3 comments)

Nir, were do you see a thread-safety issue during init?

For your second point, I do not see a real contention issue, at least not one that is justifying creation of multiple socket pools.

http://gerrit.ovirt.org/#/c/24603/3/lib/vdsm/netlink.py
File lib/vdsm/netlink.py:

Line 109:     @contextmanager
Line 110:     def socket(self):
Line 111:         """Takes a netlink socket from the pool and returns it afterwards."""
Line 112:         with self._semaphore:
Line 113:             sock = self._sockets.pop()
it would be even nicer to _open_socket() only now, and only if pop() raised KeyError - then there's no need for _get_sockets() during __init__.
Line 114:             try:
Line 115:                 yield sock
Line 116:             finally:
Line 117:                 self._sockets.add(sock)


Line 124: def _get_pool():
Line 125:     """Get (and create if necessary) the link pool."""
Line 126:     global _pool
Line 127:     if _pool is None:
Line 128:         if _pool_lock.acquire(False):
I find the standard idiom of (quick check, lock, safe check) clearer

  if _pool is None:
    with _pool_lock:
       if _pool is None:
         _pool = ...
  return _pool

it is equivalent, and we have it spread through our code.
Line 129:             try:
Line 130:                 _pool = NLSocketPool(_POOL_SIZE)
Line 131:             finally:
Line 132:                 _pool_lock.release()


Line 132:                 _pool_lock.release()
Line 133:         else:
Line 134:             with _pool_lock:
Line 135:                 pass
Line 136:     return _pool
> This lazy creation of the pool is the same code use to lazy-populate the po
In my opinion, it is actually better to have the standard singlton pattern external to the class definition.

Also, I find it nicer to avoid taking resources implicitly during import - I had an unpleasant reminder few months ago: http://gerrit.ovirt.org/22322
Line 137: 
Line 138: 
Line 139: def _open_socket():
Line 140:     """Returns an open netlink socket."""


-- 
To view, visit http://gerrit.ovirt.org/24603
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I657ac3d3e0c2661ce73bdef9aa807ead888a42eb
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list